rpms/krb5
6
0
Fork 0
Code Issues Pull Requests Releases Activity

Prefer TCP to UDP for password changes

Browse Source 
Resolves: #1637611
This commit is contained in:
Robbie Harwood 2018-10-15 13:26:07 -04:00
parent 4a2dfb104c
commit 0eeac3abaf
3 changed files with 174 additions and 401 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

396
Fix-bugs-with-concurrent-use-of-MEMORY-ccaches.patch
View File

@ -1,396 +0,0 @@
From f61875dc7da3d5dadb935ebcce25fe66564f7d0f Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
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;
}

168
Prefer-TCP-to-UDP-for-password-changes.patch Normal file
View File

@ -0,0 +1,168 @@
From dd40cfaf0eef43157afed58795e78de0bb7142eb Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
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)

11
krb5.spec
View File

@ -18,7 +18,7 @@ Summary: The Kerberos network authentication system
Name: krb5 Name: krb5
Version: 1.16.1 Version: 1.16.1
# for prerelease, should be e.g., 0.% {prerelease}.1% { ?dist } (without spaces) # 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 # lookaside-cached sources; two downloads and a build artifact
Source0: https://web.mit.edu/kerberos/dist/krb5/1.16/krb5-%{version}%{prerelease}.tar.gz 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 Patch84: Remove-outdated-note-in-krb5kdc-man-page.patch
Patch85: Fix-k5test-prompts-for-Python-3.patch Patch85: Fix-k5test-prompts-for-Python-3.patch
Patch86: In-FIPS-mode-add-plaintext-fallback-for-RC4-usages-a.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 Patch87: Prefer-TCP-to-UDP-for-password-changes.patch
# (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
License: MIT License: MIT
URL: http://web.mit.edu/kerberos/www/ URL: http://web.mit.edu/kerberos/www/
@ -753,6 +750,10 @@ exit 0
%{_libdir}/libkadm5srv_mit.so.* %{_libdir}/libkadm5srv_mit.so.*
%changelog %changelog
* Mon Oct 15 2018 Robbie Harwood <rharwood@redhat.com> - 1.16.1-22
- Prefer TCP to UDP for password changes
- Resolves: #1637611
* Tue Oct 09 2018 Adam Williamson <awilliam@redhat.com> - 1.16.1-21 * Tue Oct 09 2018 Adam Williamson <awilliam@redhat.com> - 1.16.1-21
- Revert the patch from -20 for now as it seems to make FreeIPA worse - Revert the patch from -20 for now as it seems to make FreeIPA worse