diff --git a/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch b/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch deleted file mode 100644 index 085030a..0000000 --- a/Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch +++ /dev/null @@ -1,396 +0,0 @@ -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/Prefer-TCP-to-UDP-for-password-changes.patch b/Prefer-TCP-to-UDP-for-password-changes.patch new file mode 100644 index 0000000..6df2bc1 --- /dev/null +++ b/Prefer-TCP-to-UDP-for-password-changes.patch @@ -0,0 +1,168 @@ +From dd40cfaf0eef43157afed58795e78de0bb7142eb Mon Sep 17 00:00:00 2001 +From: Robbie Harwood +Date: Mon, 8 Oct 2018 16:02:12 -0400 +Subject: [PATCH] Prefer TCP to UDP for password changes + +When password changes are performed over UDP, spotty networks may +cause the client to retransmit. This leads to replay errors if the +kpasswd server receives both requests, which hide the actual request +status and make it appear that the password has not been changed, when +it may in fact have been. Use TCP instead with UDP fallback to avoid +this issue. + +ticket: 7905 +(cherry picked from commit d7b3018d338fc9c989c3fa17505870f23c3759a8) +--- + src/lib/krb5/os/changepw.c | 110 ++++++++++++++----------------------- + 1 file changed, 42 insertions(+), 68 deletions(-) + +diff --git a/src/lib/krb5/os/changepw.c b/src/lib/krb5/os/changepw.c +index e4db57084..9f968da7f 100644 +--- a/src/lib/krb5/os/changepw.c ++++ b/src/lib/krb5/os/changepw.c +@@ -59,13 +59,12 @@ struct sendto_callback_context { + + static krb5_error_code + locate_kpasswd(krb5_context context, const krb5_data *realm, +- struct serverlist *serverlist, krb5_boolean no_udp) ++ struct serverlist *serverlist) + { + krb5_error_code code; + + code = k5_locate_server(context, realm, serverlist, locate_service_kpasswd, +- no_udp); +- ++ FALSE); + if (code == KRB5_REALM_CANT_RESOLVE || code == KRB5_REALM_UNKNOWN) { + code = k5_locate_server(context, realm, serverlist, + locate_service_kadmin, TRUE); +@@ -76,7 +75,7 @@ locate_kpasswd(krb5_context context, const krb5_data *realm, + for (i = 0; i < serverlist->nservers; i++) { + struct server_entry *s = &serverlist->servers[i]; + +- if (!no_udp && s->transport == TCP) ++ if (s->transport == TCP) + s->transport = TCP_OR_UDP; + if (s->hostname != NULL) + s->port = DEFAULT_KPASSWD_PORT; +@@ -214,7 +213,6 @@ change_set_password(krb5_context context, + krb5_data *result_string) + { + krb5_data chpw_rep; +- krb5_boolean no_udp = FALSE; + GETSOCKNAME_ARG3_TYPE addrlen; + krb5_error_code code = 0; + char *code_string; +@@ -246,73 +244,49 @@ change_set_password(krb5_context context, + callback_ctx.remote_seq_num = callback_ctx.auth_context->remote_seq_number; + callback_ctx.local_seq_num = callback_ctx.auth_context->local_seq_number; + +- do { +- k5_transport_strategy strategy = no_udp ? NO_UDP : UDP_FIRST; ++ code = locate_kpasswd(callback_ctx.context, &creds->server->realm, &sl); ++ if (code) ++ goto cleanup; + +- code = locate_kpasswd(callback_ctx.context, &creds->server->realm, &sl, +- no_udp); ++ addrlen = sizeof(remote_addr); ++ ++ callback_info.data = &callback_ctx; ++ callback_info.pfn_callback = kpasswd_sendto_msg_callback; ++ callback_info.pfn_cleanup = kpasswd_sendto_msg_cleanup; ++ krb5_free_data_contents(callback_ctx.context, &chpw_rep); ++ ++ code = k5_sendto(callback_ctx.context, NULL, &creds->server->realm, ++ &sl, UDP_LAST, &callback_info, &chpw_rep, ++ ss2sa(&remote_addr), &addrlen, NULL, NULL, NULL); ++ if (code) ++ goto cleanup; ++ ++ code = krb5int_rd_chpw_rep(callback_ctx.context, ++ callback_ctx.auth_context, ++ &chpw_rep, &local_result_code, ++ result_string); ++ ++ if (code) ++ goto cleanup; ++ ++ if (result_code) ++ *result_code = local_result_code; ++ ++ if (result_code_string) { ++ code = krb5_chpw_result_code_string(callback_ctx.context, ++ local_result_code, ++ &code_string); + if (code) +- break; ++ goto cleanup; + +- addrlen = sizeof(remote_addr); +- +- callback_info.data = &callback_ctx; +- callback_info.pfn_callback = kpasswd_sendto_msg_callback; +- callback_info.pfn_cleanup = kpasswd_sendto_msg_cleanup; +- krb5_free_data_contents(callback_ctx.context, &chpw_rep); +- +- code = k5_sendto(callback_ctx.context, NULL, &creds->server->realm, +- &sl, strategy, &callback_info, &chpw_rep, +- ss2sa(&remote_addr), &addrlen, NULL, NULL, NULL); +- if (code) { +- /* +- * Here we may want to switch to TCP on some errors. +- * right? +- */ +- break; ++ result_code_string->length = strlen(code_string); ++ result_code_string->data = malloc(result_code_string->length); ++ if (result_code_string->data == NULL) { ++ code = ENOMEM; ++ goto cleanup; + } +- +- code = krb5int_rd_chpw_rep(callback_ctx.context, +- callback_ctx.auth_context, +- &chpw_rep, &local_result_code, +- result_string); +- +- if (code) { +- if (code == KRB5KRB_ERR_RESPONSE_TOO_BIG && !no_udp) { +- k5_free_serverlist(&sl); +- no_udp = 1; +- continue; +- } +- +- break; +- } +- +- if (result_code) +- *result_code = local_result_code; +- +- if (result_code_string) { +- code = krb5_chpw_result_code_string(callback_ctx.context, +- local_result_code, +- &code_string); +- if (code) +- goto cleanup; +- +- result_code_string->length = strlen(code_string); +- result_code_string->data = malloc(result_code_string->length); +- if (result_code_string->data == NULL) { +- code = ENOMEM; +- goto cleanup; +- } +- strncpy(result_code_string->data, code_string, result_code_string->length); +- } +- +- if (code == KRB5KRB_ERR_RESPONSE_TOO_BIG && !no_udp) { +- k5_free_serverlist(&sl); +- no_udp = 1; +- } else { +- break; +- } +- } while (TRUE); ++ strncpy(result_code_string->data, code_string, result_code_string->length); ++ } + + cleanup: + if (callback_ctx.auth_context != NULL) diff --git a/krb5.spec b/krb5.spec index 9d0e2e7..0de5b73 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: 21%{?dist} +Release: 22%{?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,10 +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 -# Disabled for now as it seems to make things worse for FreeIPA -# (consistent crashes during server deployment, not just a crash -# in a later test): https://bugzilla.redhat.com/show_bug.cgi?id=1633089#c26 -#Patch87: Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch +Patch87: Prefer-TCP-to-UDP-for-password-changes.patch License: MIT URL: http://web.mit.edu/kerberos/www/ @@ -753,6 +750,10 @@ exit 0 %{_libdir}/libkadm5srv_mit.so.* %changelog +* Mon Oct 15 2018 Robbie Harwood - 1.16.1-22 +- Prefer TCP to UDP for password changes +- Resolves: #1637611 + * Tue Oct 09 2018 Adam Williamson - 1.16.1-21 - Revert the patch from -20 for now as it seems to make FreeIPA worse