diff --git a/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch b/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch new file mode 100644 index 0000000..085030a --- /dev/null +++ b/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch @@ -0,0 +1,396 @@ +From f61875dc7da3d5dadb935ebcce25fe66564f7d0f Mon Sep 17 00:00:00 2001 +From: Greg Hudson +Date: Sun, 1 Jul 2018 00:12:25 -0400 +Subject: [PATCH] Fix bugs with concurrent use of MEMORY ccaches + +A memory ccache iterator stores an alias into the cache object's +linked list of credentials. If the cache is reinitialized while the +iterator is active, the alias becomes invalid. Also, multiple handles +referencing the same memory ccache all use aliases to the same data +object; if one of the handles is destroyed, the other contains a +dangling pointer. + +Fix the first issue by adding a generation counter to the cache and to +cursors, incremented each time the cache is initialized or destroyed. +Check the generation on each cursor step and end the iteration if the +list was invalidated. Fix the second issue by adding a reference +count to the cache object, counting one reference for the table slot +and one for each open handle. Empty the cache object on each destroy +operation, but only release the object when the last handle to it is +destroyed or closed. + +Add regression tests for the two issues to t_cc.c. + +The first issue was reported by Sorin Manolache. + +ticket: 8202 +tags: pullup +target_version: 1.16-next +target_version: 1.15-next + +(cherry picked from commit 146dadec8fe7ccc4149eb2e3f577cc320aee6efb) +--- + src/lib/krb5/ccache/cc_memory.c | 164 ++++++++++++++++++++------------ + src/lib/krb5/ccache/t_cc.c | 51 ++++++++++ + 2 files changed, 154 insertions(+), 61 deletions(-) + +diff --git a/src/lib/krb5/ccache/cc_memory.c b/src/lib/krb5/ccache/cc_memory.c +index c5425eb3a..8cdaff7fb 100644 +--- a/src/lib/krb5/ccache/cc_memory.c ++++ b/src/lib/krb5/ccache/cc_memory.c +@@ -102,18 +102,20 @@ extern krb5_error_code krb5_change_cache (void); + typedef struct _krb5_mcc_link { + struct _krb5_mcc_link *next; + krb5_creds *creds; +-} krb5_mcc_link, *krb5_mcc_cursor; ++} krb5_mcc_link; + + /* Per-cache data header. */ + typedef struct _krb5_mcc_data { + char *name; + k5_cc_mutex lock; + krb5_principal prin; +- krb5_mcc_cursor link; ++ krb5_mcc_link *link; + krb5_timestamp changetime; + /* Time offsets for clock-skewed clients. */ + krb5_int32 time_offset; + krb5_int32 usec_offset; ++ int refcount; /* One for the table slot, one per handle */ ++ int generation; /* Incremented at each initialize */ + } krb5_mcc_data; + + /* List of memory caches. */ +@@ -122,6 +124,12 @@ typedef struct krb5_mcc_list_node { + krb5_mcc_data *cache; + } krb5_mcc_list_node; + ++/* Iterator over credentials in a memory cache. */ ++struct mcc_cursor { ++ int generation; ++ krb5_mcc_link *next_link; ++}; ++ + /* Iterator over memory caches. */ + struct krb5_mcc_ptcursor_data { + struct krb5_mcc_list_node *cur; +@@ -132,7 +140,23 @@ static krb5_mcc_list_node *mcc_head = 0; + + static void update_mcc_change_time(krb5_mcc_data *); + +-static void krb5_mcc_free (krb5_context context, krb5_ccache id); ++/* Remove creds from d, invalidate any existing cursors, and unset the client ++ * principal. The caller is responsible for locking. */ ++static void ++empty_mcc_cache(krb5_context context, krb5_mcc_data *d) ++{ ++ krb5_mcc_link *curr, *next; ++ ++ for (curr = d->link; curr != NULL; curr = next) { ++ next = curr->next; ++ krb5_free_creds(context, curr->creds); ++ free(curr); ++ } ++ d->link = NULL; ++ d->generation++; ++ krb5_free_principal(context, d->prin); ++ d->prin = NULL; ++} + + /* + * Modifies: +@@ -150,16 +174,12 @@ krb5_mcc_initialize(krb5_context context, krb5_ccache id, krb5_principal princ) + { + krb5_os_context os_ctx = &context->os_context; + krb5_error_code ret; +- krb5_mcc_data *d; ++ krb5_mcc_data *d = id->data; + +- d = (krb5_mcc_data *)id->data; + k5_cc_mutex_lock(context, &d->lock); ++ empty_mcc_cache(context, d); + +- krb5_mcc_free(context, id); +- +- d = (krb5_mcc_data *)id->data; +- ret = krb5_copy_principal(context, princ, +- &d->prin); ++ ret = krb5_copy_principal(context, princ, &d->prin); + update_mcc_change_time(d); + + if (os_ctx->os_flags & KRB5_OS_TOFFSET_VALID) { +@@ -185,61 +205,59 @@ krb5_mcc_initialize(krb5_context context, krb5_ccache id, krb5_principal princ) + krb5_error_code KRB5_CALLCONV + krb5_mcc_close(krb5_context context, krb5_ccache id) + { ++ krb5_mcc_data *d = id->data; ++ int count; ++ + free(id); +- return KRB5_OK; +-} +- +-static void +-krb5_mcc_free(krb5_context context, krb5_ccache id) +-{ +- krb5_mcc_cursor curr,next; +- krb5_mcc_data *d; +- +- d = (krb5_mcc_data *) id->data; +- for (curr = d->link; curr;) { +- krb5_free_creds(context, curr->creds); +- next = curr->next; +- free(curr); +- curr = next; ++ k5_cc_mutex_lock(context, &d->lock); ++ count = --d->refcount; ++ k5_cc_mutex_unlock(context, &d->lock); ++ if (count == 0) { ++ /* This is the last active handle referencing d and d has been removed ++ * from the table, so we can release it. */ ++ empty_mcc_cache(context, d); ++ free(d->name); ++ k5_cc_mutex_destroy(&d->lock); ++ free(d); + } +- d->link = NULL; +- krb5_free_principal(context, d->prin); ++ return KRB5_OK; + } + + /* + * Effects: + * Destroys the contents of id. id is invalid after call. +- * +- * Errors: +- * system errors (locks related) + */ + krb5_error_code KRB5_CALLCONV + krb5_mcc_destroy(krb5_context context, krb5_ccache id) + { + krb5_mcc_list_node **curr, *node; +- krb5_mcc_data *d; ++ krb5_mcc_data *d = id->data; ++ krb5_boolean removed_from_table = FALSE; + + k5_cc_mutex_lock(context, &krb5int_mcc_mutex); + +- d = (krb5_mcc_data *)id->data; + for (curr = &mcc_head; *curr; curr = &(*curr)->next) { + if ((*curr)->cache == d) { + node = *curr; + *curr = node->next; + free(node); ++ removed_from_table = TRUE; + break; + } + } + k5_cc_mutex_unlock(context, &krb5int_mcc_mutex); + ++ /* Empty the cache and remove the reference for the table slot. There will ++ * always be at least one reference left for the handle being destroyed. */ + k5_cc_mutex_lock(context, &d->lock); +- +- krb5_mcc_free(context, id); +- free(d->name); ++ empty_mcc_cache(context, d); ++ if (removed_from_table) ++ d->refcount--; + k5_cc_mutex_unlock(context, &d->lock); +- k5_cc_mutex_destroy(&d->lock); +- free(d); +- free(id); ++ ++ /* Invalidate the handle, possibly removing the last reference to d and ++ * freeing it. */ ++ krb5_mcc_close(context, id); + + krb5_change_cache (); + return KRB5_OK; +@@ -279,9 +297,12 @@ krb5_mcc_resolve (krb5_context context, krb5_ccache *id, const char *residual) + for (ptr = mcc_head; ptr; ptr=ptr->next) + if (!strcmp(ptr->cache->name, residual)) + break; +- if (ptr) ++ if (ptr != NULL) { + d = ptr->cache; +- else { ++ k5_cc_mutex_lock(context, &d->lock); ++ d->refcount++; ++ k5_cc_mutex_unlock(context, &d->lock); ++ } else { + err = new_mcc_data(residual, &d); + if (err) { + k5_cc_mutex_unlock(context, &krb5int_mcc_mutex); +@@ -326,14 +347,18 @@ krb5_error_code KRB5_CALLCONV + krb5_mcc_start_seq_get(krb5_context context, krb5_ccache id, + krb5_cc_cursor *cursor) + { +- krb5_mcc_cursor mcursor; ++ struct mcc_cursor *mcursor; + krb5_mcc_data *d; + ++ mcursor = malloc(sizeof(*mcursor)); ++ if (mcursor == NULL) ++ return KRB5_CC_NOMEM; + d = id->data; + k5_cc_mutex_lock(context, &d->lock); +- mcursor = d->link; ++ mcursor->generation = d->generation; ++ mcursor->next_link = d->link; + k5_cc_mutex_unlock(context, &d->lock); +- *cursor = (krb5_cc_cursor) mcursor; ++ *cursor = mcursor; + return KRB5_OK; + } + +@@ -361,23 +386,34 @@ krb5_error_code KRB5_CALLCONV + krb5_mcc_next_cred(krb5_context context, krb5_ccache id, + krb5_cc_cursor *cursor, krb5_creds *creds) + { +- krb5_mcc_cursor mcursor; ++ struct mcc_cursor *mcursor; + krb5_error_code retval; ++ krb5_mcc_data *d = id->data; + +- /* Once the node in the linked list is created, it's never +- modified, so we don't need to worry about locking here. (Note +- that we don't support _remove_cred.) */ +- mcursor = (krb5_mcc_cursor) *cursor; +- if (mcursor == NULL) +- return KRB5_CC_END; + memset(creds, 0, sizeof(krb5_creds)); +- if (mcursor->creds) { +- retval = k5_copy_creds_contents(context, mcursor->creds, creds); +- if (retval) +- return retval; ++ mcursor = *cursor; ++ if (mcursor->next_link == NULL) ++ return KRB5_CC_END; ++ ++ /* ++ * Check the cursor generation against the cache generation in case the ++ * cache has been reinitialized or destroyed, freeing the pointer in the ++ * cursor. Keep the cache locked while we copy the creds and advance the ++ * pointer, in case another thread reinitializes the cache after we check ++ * the generation. ++ */ ++ k5_cc_mutex_lock(context, &d->lock); ++ if (mcursor->generation != d->generation) { ++ k5_cc_mutex_unlock(context, &d->lock); ++ return KRB5_CC_END; + } +- *cursor = (krb5_cc_cursor)mcursor->next; +- return KRB5_OK; ++ ++ retval = k5_copy_creds_contents(context, mcursor->next_link->creds, creds); ++ if (retval == 0) ++ mcursor->next_link = mcursor->next_link->next; ++ ++ k5_cc_mutex_unlock(context, &d->lock); ++ return retval; + } + + /* +@@ -396,14 +432,18 @@ krb5_mcc_next_cred(krb5_context context, krb5_ccache id, + krb5_error_code KRB5_CALLCONV + krb5_mcc_end_seq_get(krb5_context context, krb5_ccache id, krb5_cc_cursor *cursor) + { +- *cursor = 0L; ++ free(*cursor); ++ *cursor = NULL; + return KRB5_OK; + } + +-/* Utility routine: Creates the back-end data for a memory cache, and +- threads it into the global linked list. +- +- Call with the global list lock held. */ ++/* ++ * Utility routine: Creates the back-end data for a memory cache, and threads ++ * it into the global linked list. Give the new object two references, one for ++ * the table slot and one for the caller's handle. ++ * ++ * Call with the global list lock held. ++ */ + static krb5_error_code + new_mcc_data (const char *name, krb5_mcc_data **dataptr) + { +@@ -432,6 +472,8 @@ new_mcc_data (const char *name, krb5_mcc_data **dataptr) + d->changetime = 0; + d->time_offset = 0; + d->usec_offset = 0; ++ d->refcount = 2; ++ d->generation = 0; + update_mcc_change_time(d); + + n = malloc(sizeof(krb5_mcc_list_node)); +diff --git a/src/lib/krb5/ccache/t_cc.c b/src/lib/krb5/ccache/t_cc.c +index 6069cabd3..cd4569c4c 100644 +--- a/src/lib/krb5/ccache/t_cc.c ++++ b/src/lib/krb5/ccache/t_cc.c +@@ -386,6 +386,55 @@ test_misc(krb5_context context) + krb5_cc_dfl_ops = ops_save; + + } ++ ++/* ++ * Regression tests for #8202. Because memory ccaches share objects between ++ * different handles to the same cache and between iterators and caches, ++ * historically there have been some bugs when those objects are released. ++ */ ++static void ++test_memory_concurrent(krb5_context context) ++{ ++ krb5_error_code kret; ++ krb5_ccache id1, id2; ++ krb5_cc_cursor cursor; ++ krb5_creds creds; ++ ++ /* Create two handles to the same memory ccache and destroy them. */ ++ kret = krb5_cc_resolve(context, "MEMORY:x", &id1); ++ CHECK(kret, "resolve 1"); ++ kret = krb5_cc_resolve(context, "MEMORY:x", &id2); ++ CHECK(kret, "resolve 2"); ++ kret = krb5_cc_destroy(context, id1); ++ CHECK(kret, "destroy 1"); ++ kret = krb5_cc_destroy(context, id2); ++ CHECK(kret, "destroy 2"); ++ ++ kret = init_test_cred(context); ++ CHECK(kret, "init_creds"); ++ ++ /* Reinitialize the cache after creating an iterator for it, and verify ++ * that the iterator ends gracefully. */ ++ kret = krb5_cc_resolve(context, "MEMORY:x", &id1); ++ CHECK(kret, "resolve"); ++ kret = krb5_cc_initialize(context, id1, test_creds.client); ++ CHECK(kret, "initialize"); ++ kret = krb5_cc_store_cred(context, id1, &test_creds); ++ CHECK(kret, "store"); ++ kret = krb5_cc_start_seq_get(context, id1, &cursor); ++ CHECK(kret, "start_seq_get"); ++ kret = krb5_cc_initialize(context, id1, test_creds.client); ++ CHECK(kret, "initialize again"); ++ kret = krb5_cc_next_cred(context, id1, &cursor, &creds); ++ CHECK_BOOL(kret != KRB5_CC_END, "iterator should end", "next_cred"); ++ kret = krb5_cc_end_seq_get(context, id1, &cursor); ++ CHECK(kret, "end_seq_get"); ++ kret = krb5_cc_destroy(context, id1); ++ CHECK(kret, "destroy"); ++ ++ free_test_cred(context); ++} ++ + extern const krb5_cc_ops krb5_mcc_ops; + extern const krb5_cc_ops krb5_fcc_ops; + +@@ -434,6 +483,8 @@ main(void) + do_test(context, "MEMORY:"); + do_test(context, "FILE:"); + ++ test_memory_concurrent(context); ++ + krb5_free_context(context); + return 0; + } diff --git a/krb5.spec b/krb5.spec index ae797b0..12f7d60 100644 --- a/krb5.spec +++ b/krb5.spec @@ -18,7 +18,7 @@ Summary: The Kerberos network authentication system Name: krb5 Version: 1.16.1 # for prerelease, should be e.g., 0.% {prerelease}.1% { ?dist } (without spaces) -Release: 19%{?dist} +Release: 20%{?dist} # lookaside-cached sources; two downloads and a build artifact Source0: https://web.mit.edu/kerberos/dist/krb5/1.16/krb5-%{version}%{prerelease}.tar.gz @@ -103,6 +103,7 @@ Patch83: Make-krb5kdc-p-affect-TCP-ports.patch Patch84: Remove-outdated-note-in-krb5kdc-man-page.patch Patch85: Fix-k5test-prompts-for-Python-3.patch Patch86: In-FIPS-mode-add-plaintext-fallback-for-RC4-usages-a.patch +Patch87: Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch License: MIT URL: http://web.mit.edu/kerberos/www/ @@ -749,6 +750,9 @@ exit 0 %{_libdir}/libkadm5srv_mit.so.* %changelog +* Tue Oct 02 2018 Robbie Harwood - 1.16.1-20 +- Fix bugs with concurrent use of MEMORY ccaches + * Wed Aug 01 2018 Robbie Harwood - 1.16.1-19 - In FIPS mode, add plaintext fallback for RC4 usages and taint