From ec84667763547219241b79547b4d186b8b0c4787 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Mon, 9 Sep 2019 19:19:53 -0500 Subject: [PATCH] Fix krb5_cc_move() issues Move init/copy/destroy fallback sequence from fcc_move() to krb5_cc_move(). Make sure all backends's move() method calls krb5_cc_destroy() on the source on success (and only on success). In text_cc make sure that we can find in the destination the cred stored into the source. --- lib/krb5/acache.c | 3 +- lib/krb5/cache.c | 47 +++++++++++++++++++-------- lib/krb5/fcache.c | 80 +++++----------------------------------------- lib/krb5/kcm.c | 3 ++ lib/krb5/krcache.c | 5 +-- lib/krb5/mcache.c | 2 +- lib/krb5/scache.c | 12 ++----- lib/krb5/test_cc.c | 28 +++++++++++++--- 8 files changed, 73 insertions(+), 107 deletions(-) diff --git a/lib/krb5/acache.c b/lib/krb5/acache.c index 4cadb5de5..4f6fa3c0b 100644 --- a/lib/krb5/acache.c +++ b/lib/krb5/acache.c @@ -992,8 +992,7 @@ acc_move(krb5_context context, krb5_ccache from, krb5_ccache to) error = (*ato->ccache->func->move)(afrom->ccache, ato->ccache); - acc_destroy(context, from); - + krb5_cc_destroy(context, from); return translate_cc_error(context, error); } diff --git a/lib/krb5/cache.c b/lib/krb5/cache.c index 274dd8026..a206f75c2 100644 --- a/lib/krb5/cache.c +++ b/lib/krb5/cache.c @@ -1295,8 +1295,9 @@ krb5_cc_cache_match (krb5_context context, * @param from the credential cache to move the content from * @param to the credential cache to move the content to - * @return On sucess, from is freed. On failure, error code is - * returned and from and to are both still allocated, see krb5_get_error_message(). + * @return On sucess, from is destroyed and closed. On failure, error code is + * returned and from and to are both still allocated; see + * krb5_get_error_message(). * * @ingroup krb5_ccache */ @@ -1304,20 +1305,38 @@ krb5_cc_cache_match (krb5_context context, KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL krb5_cc_move(krb5_context context, krb5_ccache from, krb5_ccache to) { - krb5_error_code ret; + krb5_error_code ret = ENOTSUP; + krb5_principal princ = NULL; - if (strcmp(from->ops->prefix, to->ops->prefix) != 0) { - krb5_set_error_message(context, KRB5_CC_NOSUPP, - N_("Moving credentials between diffrent " - "types not yet supported", "")); - return KRB5_CC_NOSUPP; - } + if (to->ops->move && + strcmp(from->ops->prefix, to->ops->prefix) == 0) { + /* + * NOTE: to->ops->move() is expected to call + * krb5_cc_destroy(context, from) on success. + */ + ret = (*to->ops->move)(context, from, to); + if (ret == 0) + return 0; + if (ret != EXDEV && ret != ENOTSUP) + return ret; + /* Fallback to high-level copy */ + } /* Else high-level copy */ - ret = (*to->ops->move)(context, from, to); - if (ret == 0) { - memset(from, 0, sizeof(*from)); - free(from); - } + /* + * Initialize destination, copy the source's contents to the destination, + * then destroy the source on success. + * + * It'd be nice if we could destroy any half-built destination if the copy + * fails, but the interface is not documented as doing so. + */ + ret = krb5_cc_get_principal(context, from, &princ); + if (ret == 0) + ret = krb5_cc_initialize(context, to, princ); + krb5_free_principal(context, princ); + if (ret == 0) + ret = krb5_cc_copy_cache(context, from, to); + if (ret == 0) + krb5_cc_destroy(context, from); return ret; } diff --git a/lib/krb5/fcache.c b/lib/krb5/fcache.c index e93c2f2e0..a0963ceb2 100644 --- a/lib/krb5/fcache.c +++ b/lib/krb5/fcache.c @@ -1160,78 +1160,14 @@ fcc_move(krb5_context context, krb5_ccache from, krb5_ccache to) { krb5_error_code ret = 0; - ret = rk_rename(FILENAME(from), FILENAME(to)); - - if (ret && errno != EXDEV) { - char buf[128]; - ret = errno; - rk_strerror_r(ret, buf, sizeof(buf)); - krb5_set_error_message(context, ret, - N_("Rename of file from %s " - "to %s failed: %s", ""), - FILENAME(from), FILENAME(to), buf); - return ret; - } else if (ret && errno == EXDEV) { - /* make a copy and delete the orignal */ - krb5_ssize_t sz1, sz2; - int fd1, fd2; - char buf[BUFSIZ]; - - ret = fcc_open(context, from, "move/from", &fd1, O_RDONLY, 0); - if(ret) - return ret; - - unlink(FILENAME(to)); - - ret = fcc_open(context, to, "move/to", &fd2, - O_WRONLY | O_CREAT | O_EXCL, 0600); - if(ret) - goto out1; - - while((sz1 = read(fd1, buf, sizeof(buf))) > 0) { - sz2 = write(fd2, buf, sz1); - if (sz1 != sz2) { - ret = EIO; - krb5_set_error_message(context, ret, - N_("Failed to write data from one file " - "credential cache to the other", "")); - goto out2; - } - } - if (sz1 < 0) { - ret = EIO; - krb5_set_error_message(context, ret, - N_("Failed to read data from one file " - "credential cache to the other", "")); - goto out2; - } - out2: - close(fd2); - - out1: - close(fd1); - - _krb5_erase_file(context, FILENAME(from)); - - if (ret) { - _krb5_erase_file(context, FILENAME(to)); - return ret; - } - } - - /* make sure ->version is uptodate */ - { - krb5_storage *sp; - int fd; - if ((ret = init_fcc (context, to, "move", &sp, &fd, NULL)) == 0) { - if (sp) - krb5_storage_free(sp); - close(fd); - } - } - - fcc_close(context, from); - + if ((ret = rk_rename(FILENAME(from), FILENAME(to)))) + ret = errno; + /* + * We need only close from -- we can't destroy it since the rename + * succeeded, which "destroyed" it at its old name. + */ + if (ret == 0) + krb5_cc_close(context, from); return ret; } diff --git a/lib/krb5/kcm.c b/lib/krb5/kcm.c index 8e1dfff2d..6f48ff4ca 100644 --- a/lib/krb5/kcm.c +++ b/lib/krb5/kcm.c @@ -944,6 +944,9 @@ kcm_move(krb5_context context, krb5_ccache from, krb5_ccache to) ret = krb5_kcm_call(context, request, NULL, NULL); krb5_storage_free(request); + + if (ret == 0) + krb5_cc_destroy(context, from); return ret; } diff --git a/lib/krb5/krcache.c b/lib/krb5/krcache.c index 72fce21ad..1d425ca3e 100644 --- a/lib/krb5/krcache.c +++ b/lib/krb5/krcache.c @@ -1977,10 +1977,7 @@ krcc_move(krb5_context context, krb5_ccache from, krb5_ccache to) } update_change_time(context, now, krto); - - krcc_destroy(context, from); - krcc_close(context, from); - + krb5_cc_destroy(context, from); return 0; } diff --git a/lib/krb5/mcache.c b/lib/krb5/mcache.c index e45bc1b0a..a8a638b16 100644 --- a/lib/krb5/mcache.c +++ b/lib/krb5/mcache.c @@ -519,8 +519,8 @@ mcc_move(krb5_context context, krb5_ccache from, krb5_ccache to) HEIMDAL_MUTEX_unlock(&(mfrom->mutex)); HEIMDAL_MUTEX_unlock(&(mto->mutex)); HEIMDAL_MUTEX_unlock(&mcc_mutex); - mcc_destroy(context, from); + krb5_cc_destroy(context, from); return 0; } diff --git a/lib/krb5/scache.c b/lib/krb5/scache.c index 61a9b4fa1..d3b9764c0 100644 --- a/lib/krb5/scache.c +++ b/lib/krb5/scache.c @@ -1276,11 +1276,8 @@ scc_move(krb5_context context, krb5_ccache from, krb5_ccache to) krb5_error_code ret; if (strcmp(sfrom->file, sto->file) != 0) { - krb5_set_error_message(context, KRB5_CC_BADNAME, - N_("Can't handle cross database " - "credential move: %s -> %s", ""), - sfrom->file, sto->file); - return KRB5_CC_BADNAME; + /* Let upstairs handle the move */ + return EXDEV; } ret = make_database(context, sfrom); @@ -1326,14 +1323,11 @@ scc_move(krb5_context context, krb5_ccache from, krb5_ccache to) ret = exec_stmt(context, sfrom->db, "COMMIT", KRB5_CC_IO); if (ret) return ret; - scc_free(sfrom); - + krb5_cc_close(context, from); return 0; rollback: exec_stmt(context, sfrom->db, "ROLLBACK", 0); - scc_free(sfrom); - return KRB5_CC_IO; } diff --git a/lib/krb5/test_cc.c b/lib/krb5/test_cc.c index c1d40f702..f29a1ba5b 100644 --- a/lib/krb5/test_cc.c +++ b/lib/krb5/test_cc.c @@ -249,6 +249,8 @@ test_cache_remove(krb5_context context, const char *type) if (ret) krb5_err(context, 1, ret, "krb5_cc_remove_cred"); + /* XXX Search for it */ + ret = krb5_cc_destroy(context, id); if (ret) krb5_err(context, 1, ret, "krb5_cc_destroy"); @@ -506,6 +508,7 @@ test_move(krb5_context context, const char *type) krb5_ccache fromid, toid; krb5_error_code ret; krb5_principal p, p2; + krb5_creds cred, tocred; ops = krb5_cc_get_prefix_ops(context, type); if (ops == NULL) @@ -525,13 +528,24 @@ test_move(krb5_context context, const char *type) if (ret) krb5_err(context, 1, ret, "krb5_cc_initialize"); + memset(&cred, 0, sizeof(cred)); + ret = krb5_parse_name(context, "krbtgt/SU.SE@SU.SE", &cred.server); + if (ret) + krb5_err(context, 1, ret, "krb5_parse_name"); + ret = krb5_parse_name(context, "lha@SU.SE", &cred.client); + if (ret) + krb5_err(context, 1, ret, "krb5_parse_name"); + + ret = krb5_cc_store_cred(context, fromid, &cred); + if (ret) + krb5_err(context, 1, ret, "krb5_cc_store_cred"); + + ret = krb5_cc_new_unique(context, type, NULL, &toid); if (ret) krb5_err(context, 1, ret, "krb5_cc_new_unique"); - ret = krb5_cc_initialize(context, toid, p); - if (ret) - krb5_err(context, 1, ret, "krb5_cc_initialize"); + ret = krb5_cc_move(context, fromid, toid); ret = krb5_cc_get_principal(context, toid, &p2); if (ret) @@ -540,11 +554,15 @@ test_move(krb5_context context, const char *type) if (krb5_principal_compare(context, p, p2) == FALSE) krb5_errx(context, 1, "p != p2"); + ret = krb5_cc_retrieve_cred(context, toid, 0, &cred, &tocred); + if (ret) + krb5_errx(context, 1, "move failed"); + krb5_free_cred_contents(context, &cred); + krb5_free_cred_contents(context, &tocred); + krb5_free_principal(context, p); krb5_free_principal(context, p2); - krb5_cc_destroy(context, toid); - krb5_cc_destroy(context, fromid); }