From 0ae6147483862ae41e9e81cf1b0126d726e7cdf3 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Fri, 11 Nov 2016 04:25:37 +0000 Subject: [PATCH] Fix kadm5 error cleanup --- lib/kadm5/chpass_s.c | 48 ++++++++++++++++++++++++---------------------- lib/kadm5/context_s.c | 15 ++++++++++++++- lib/kadm5/create_s.c | 30 +++++++++++++++++------------ lib/kadm5/delete_s.c | 11 ++++++----- lib/kadm5/free.c | 6 ++---- lib/kadm5/get_princs_s.c | 17 +++++++++------- lib/kadm5/get_s.c | 50 ++++++++++++++++-------------------------------- lib/kadm5/modify_s.c | 25 ++++++++++++------------ lib/kadm5/randkey_s.c | 19 +++++++++--------- lib/kadm5/rename_s.c | 24 ++++++++++++----------- lib/kadm5/setkey3_s.c | 14 +++++++++----- 11 files changed, 137 insertions(+), 122 deletions(-) diff --git a/lib/kadm5/chpass_s.c b/lib/kadm5/chpass_s.c index 4b873ed78..d1ed73207 100644 --- a/lib/kadm5/chpass_s.c +++ b/lib/kadm5/chpass_s.c @@ -64,8 +64,8 @@ change(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, HDB_F_DECRYPT|HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); - if(ret) - goto out; + if (ret) + goto out2; if (keepold || cond) { /* @@ -74,14 +74,14 @@ change(void *server_handle, */ ret = hdb_add_current_keys_to_history(context->context, &ent.entry); if (ret) - goto out; + goto out3; } if (context->db->hdb_capability_flags & HDB_CAP_F_HANDLE_PASSWORDS) { ret = context->db->hdb_password(context->context, context->db, &ent, password, cond); if (ret) - goto out2; + goto out3; } else { num_keys = ent.entry.keys.len; @@ -94,7 +94,7 @@ change(void *server_handle, password); if(ret) { _kadm5_free_keys(context->context, num_keys, keys); - goto out2; + goto out3; } _kadm5_free_keys(context->context, num_keys, keys); @@ -112,7 +112,7 @@ change(void *server_handle, ret = KADM5_PASS_REUSE; krb5_set_error_message(context->context, ret, "Password reuse forbidden"); - goto out2; + goto out3; } } ent.entry.kvno++; @@ -127,20 +127,20 @@ change(void *server_handle, ext.data.element = choice_HDB_extension_data_hist_keys; ret = hdb_replace_extension(context->context, &ent.entry, &ext); if (ret) - goto out2; + goto out3; } ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out3; ret = _kadm5_set_modifier(context, &ent.entry); if(ret) - goto out2; + goto out3; ret = _kadm5_bump_pw_expire(context, &ent.entry); if (ret) - goto out2; + goto out3; /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_modify(context, &ent.entry, @@ -149,10 +149,11 @@ change(void *server_handle, KADM5_KEY_DATA | KADM5_KVNO | KADM5_PW_EXPIRATION | KADM5_TL_DATA); -out2: + out3: hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); @@ -221,28 +222,28 @@ kadm5_s_chpass_principal_with_key(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, 0, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, &ent); - if(ret == HDB_ERR_NOENTRY) - goto out; + if (ret == HDB_ERR_NOENTRY) + goto out2; if (keepold) { ret = hdb_add_current_keys_to_history(context->context, &ent.entry); if (ret) - goto out2; + goto out3; } ret = _kadm5_set_keys2(context, &ent.entry, n_key_data, key_data); - if(ret) - goto out2; + if (ret) + goto out3; ent.entry.kvno++; ret = _kadm5_set_modifier(context, &ent.entry); - if(ret) - goto out2; + if (ret) + goto out3; ret = _kadm5_bump_pw_expire(context, &ent.entry); if (ret) - goto out2; + goto out3; if (keepold) { ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out3; } else { HDB_extension ext; @@ -260,10 +261,11 @@ kadm5_s_chpass_principal_with_key(void *server_handle, KADM5_MOD_TIME | KADM5_KEY_DATA | KADM5_KVNO | KADM5_PW_EXPIRATION | KADM5_TL_DATA); -out2: + out3: hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/context_s.c b/lib/kadm5/context_s.c index cddd1360b..2c7bed62a 100644 --- a/lib/kadm5/context_s.c +++ b/lib/kadm5/context_s.c @@ -57,8 +57,21 @@ kadm5_s_lock(void *server_handle) return ret; ret = context->db->hdb_lock(context->context, context->db, HDB_WLOCK); - if (ret) + if (ret) { + (void) context->db->hdb_close(context->context, context->db); return ret; + } + + /* + * Attempt to recover the log. This will generally fail on slaves, + * and we can't tell if we're on a slave here. + * + * Perhaps we could set a flag in the kadm5_server_context to + * indicate whether a read has been done without recovering the log, + * in which case we could fail any subsequent writes. + */ + if (kadm5_log_init(context) == 0) + (void) kadm5_log_end(context); context->keep_open = 1; return 0; diff --git a/lib/kadm5/create_s.c b/lib/kadm5/create_s.c index e74d0a017..7d8f89837 100644 --- a/lib/kadm5/create_s.c +++ b/lib/kadm5/create_s.c @@ -124,13 +124,15 @@ kadm5_s_create_principal_with_key(void *server_handle, | KADM5_AUX_ATTRIBUTES | KADM5_POLICY_CLR | KADM5_LAST_SUCCESS | KADM5_LAST_FAILED | KADM5_FAIL_AUTH_COUNT); - if(ret) - goto out; + if (ret) + return ret; if (!context->keep_open) { ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0); - if (ret) - goto out; + if (ret) { + hdb_free_entry(context->context, &ent); + return ret; + } } ret = kadm5_log_init(context); @@ -139,13 +141,14 @@ kadm5_s_create_principal_with_key(void *server_handle, ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out; + goto out2; /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_create(context, &ent.entry); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); @@ -183,12 +186,14 @@ kadm5_s_create_principal(void *server_handle, | KADM5_POLICY_CLR | KADM5_LAST_SUCCESS | KADM5_LAST_FAILED | KADM5_FAIL_AUTH_COUNT); if (ret) - goto out; + return ret; if (!context->keep_open) { ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0); - if (ret) - goto out; + if (ret) { + hdb_free_entry(context->context, &ent); + return ret; + } } ret = kadm5_log_init(context); @@ -200,17 +205,18 @@ kadm5_s_create_principal(void *server_handle, ret = _kadm5_set_keys(context, &ent.entry, n_ks_tuple, ks_tuple, password); if (ret) - goto out; + goto out2; ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out; + goto out2; /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_create(context, &ent.entry); - out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/delete_s.c b/lib/kadm5/delete_s.c index 1edbea2e0..2e4aeff53 100644 --- a/lib/kadm5/delete_s.c +++ b/lib/kadm5/delete_s.c @@ -58,23 +58,24 @@ kadm5_s_delete_principal(void *server_handle, krb5_principal princ) ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, HDB_F_DECRYPT|HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); if (ret == HDB_ERR_NOENTRY) - goto out; + goto out2; if (ent.entry.flags.immutable) { ret = KADM5_PROTECT_PRINCIPAL; - goto out2; + goto out3; } ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out3; /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_delete(context, princ); -out2: + out3: hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/free.c b/lib/kadm5/free.c index 40c0573af..e8bb0f943 100644 --- a/lib/kadm5/free.c +++ b/lib/kadm5/free.c @@ -75,10 +75,8 @@ kadm5_free_principal_ent(void *server_handle, free(tp->tl_data_contents); free(tp); } - if (princ->key_data != NULL) - free(princ->key_data); - if (princ->policy) - free(princ->policy); + free(princ->key_data); + free(princ->policy); } void diff --git a/lib/kadm5/get_princs_s.c b/lib/kadm5/get_princs_s.c index 0e8f92268..22b2091be 100644 --- a/lib/kadm5/get_princs_s.c +++ b/lib/kadm5/get_princs_s.c @@ -88,7 +88,7 @@ kadm5_s_get_principals(void *server_handle, if (!context->keep_open) { ret = context->db->hdb_open(context->context, context->db, O_RDONLY, 0); - if(ret) { + if (ret) { krb5_warn(context->context, ret, "opening database"); return ret; } @@ -102,21 +102,24 @@ kadm5_s_get_principals(void *server_handle, aret = asprintf(&d.exp2, "%s@%s", expression, r); free(r); if (aret == -1 || d.exp2 == NULL) { - return ENOMEM; + ret = ENOMEM; + goto out; } } d.princs = NULL; d.count = 0; ret = hdb_foreach(context->context, context->db, HDB_F_ADMIN_DATA, foreach, &d); - if (!context->keep_open) - context->db->hdb_close(context->context, context->db); - if(ret == 0) + + if (ret == 0) ret = add_princ(&d, NULL); - if(ret == 0){ + if (ret == 0){ *princs = d.princs; *count = d.count - 1; - }else + } else kadm5_free_name_list(context, d.princs, &d.count); free(d.exp2); + out: + if (!context->keep_open) + context->db->hdb_close(context->context, context->db); return _kadm5_error_code(ret); } diff --git a/lib/kadm5/get_s.c b/lib/kadm5/get_s.c index a05a4ff88..8f9e7e908 100644 --- a/lib/kadm5/get_s.c +++ b/lib/kadm5/get_s.c @@ -126,6 +126,7 @@ kadm5_s_get_principal(void *server_handle, int hdb_is_rw = 1; memset(&ent, 0, sizeof(ent)); + memset(out, 0, sizeof(*out)); if (!context->keep_open) { ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0); @@ -133,7 +134,7 @@ kadm5_s_get_principal(void *server_handle, ret = context->db->hdb_open(context->context, context->db, O_RDONLY, 0); hdb_is_rw = 0; } - if(ret) + if (ret) return ret; } @@ -145,21 +146,18 @@ kadm5_s_get_principal(void *server_handle, * indicate whether a read has been done without recovering the log, * in which case we could fail any subsequent writes. */ - if (hdb_is_rw) - (void) kadm5_log_init_nb(context); + if (hdb_is_rw && kadm5_log_init_nb(context) == 0) + (void) kadm5_log_end(context); ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, HDB_F_DECRYPT|HDB_F_ALL_KVNOS| HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); - if (hdb_is_rw) - kadm5_log_end(context); if (!context->keep_open) context->db->hdb_close(context->context, context->db); if(ret) return _kadm5_error_code(ret); - memset(out, 0, sizeof(*out)); if(mask & KADM5_PRINCIPAL) ret = krb5_copy_principal(context->context, ent.entry.principal, &out->principal); @@ -296,10 +294,8 @@ kadm5_s_get_principal(void *server_handle, krb5_free_salt(context->context, salt); assert( out->n_key_data == n_keys ); } - if(ret){ - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } if(mask & KADM5_TL_DATA) { time_t last_pw_expire; const HDB_Ext_PKINIT_acl *acl; @@ -311,15 +307,13 @@ kadm5_s_get_principal(void *server_handle, _krb5_put_int(buf, last_pw_expire, sizeof(buf)); ret = add_tl_data(out, KRB5_TL_LAST_PWD_CHANGE, buf, sizeof(buf)); } - if(ret){ - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } /* * If the client was allowed to get key data, let it have the * password too. */ - if(mask & KADM5_KEY_DATA) { + if (mask & KADM5_KEY_DATA) { heim_utf8_string pw; ret = hdb_entry_get_password(context->context, @@ -338,24 +332,18 @@ kadm5_s_get_principal(void *server_handle, ASN1_MALLOC_ENCODE(HDB_Ext_PKINIT_acl, buf.data, buf.length, acl, &len, ret); - if (ret) { - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } if (len != buf.length) krb5_abortx(context->context, "internal ASN.1 encoder error"); ret = add_tl_data(out, KRB5_TL_PKINIT_ACL, buf.data, buf.length); free(buf.data); - if (ret) { - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } } - if(ret){ - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } ret = hdb_entry_get_aliases(&ent.entry, &aliases); if (ret == 0 && aliases) { @@ -364,27 +352,23 @@ kadm5_s_get_principal(void *server_handle, ASN1_MALLOC_ENCODE(HDB_Ext_Aliases, buf.data, buf.length, aliases, &len, ret); - if (ret) { - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } if (len != buf.length) krb5_abortx(context->context, "internal ASN.1 encoder error"); ret = add_tl_data(out, KRB5_TL_ALIASES, buf.data, buf.length); free(buf.data); - if (ret) { - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } } - if(ret){ - kadm5_free_principal_ent(context, out); + if (ret) goto out; - } - } -out: + + out: + if (ret) + kadm5_free_principal_ent(context, out); hdb_free_entry(context->context, &ent); return _kadm5_error_code(ret); diff --git a/lib/kadm5/modify_s.c b/lib/kadm5/modify_s.c index 8d92609f1..78f26734f 100644 --- a/lib/kadm5/modify_s.c +++ b/lib/kadm5/modify_s.c @@ -64,14 +64,14 @@ modify_principal(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, princ->principal, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); - if(ret) - goto out; - ret = _kadm5_setup_entry(context, &ent, mask, princ, mask, NULL, 0); - if(ret) + if (ret) goto out2; + ret = _kadm5_setup_entry(context, &ent, mask, princ, mask, NULL, 0); + if (ret) + goto out3; ret = _kadm5_set_modifier(context, &ent.entry); - if(ret) - goto out2; + if (ret) + goto out3; /* * If any keys are bogus, disallow the modify. If the keys were @@ -84,12 +84,12 @@ modify_principal(void *server_handle, if ((mask & KADM5_KEY_DATA) && kadm5_some_keys_are_bogus(princ->n_key_data, princ->key_data)) { ret = KADM5_AUTH_GET_KEYS; /* Not quite appropriate, but it'll do */ - goto out2; + goto out3; } ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out3; if ((mask & KADM5_POLICY)) { HDB_extension ext; @@ -101,23 +101,24 @@ modify_principal(void *server_handle, ext.data.u.policy = strdup(princ->policy); if (ext.data.u.policy == NULL) { ret = ENOMEM; - goto out2; + goto out3; } /* This calls free_HDB_extension(), freeing ext.data.u.policy */ ret = hdb_replace_extension(context->context, &ent.entry, &ext); free(ext.data.u.policy); if (ret) - goto out2; + goto out3; } /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_modify(context, &ent.entry, mask | KADM5_MOD_NAME | KADM5_MOD_TIME); -out2: + out3: hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/randkey_s.c b/lib/kadm5/randkey_s.c index e1308fd88..64578ab49 100644 --- a/lib/kadm5/randkey_s.c +++ b/lib/kadm5/randkey_s.c @@ -67,33 +67,33 @@ kadm5_s_randkey_principal(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); if(ret) - goto out; + goto out2; if (keepold) { ret = hdb_add_current_keys_to_history(context->context, &ent.entry); if (ret) - goto out2; + goto out3; } ret = _kadm5_set_keys_randomly(context, &ent.entry, n_ks_tuple, ks_tuple, new_keys, n_keys); if (ret) - goto out2; + goto out3; ent.entry.kvno++; ent.entry.flags.require_pwchange = 0; ret = _kadm5_set_modifier(context, &ent.entry); if(ret) - goto out3; + goto out4; ret = _kadm5_bump_pw_expire(context, &ent.entry); if (ret) - goto out2; + goto out4; if (keepold) { ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out4; } else { HDB_extension ext; @@ -112,7 +112,7 @@ kadm5_s_randkey_principal(void *server_handle, KADM5_KEY_DATA | KADM5_KVNO | KADM5_PW_EXPIRATION | KADM5_TL_DATA); -out3: + out4: if (ret) { int i; @@ -122,10 +122,11 @@ out3: *new_keys = NULL; *n_keys = 0; } -out2: + out3: hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/rename_s.c b/lib/kadm5/rename_s.c index 9b8019840..16a5e11c1 100644 --- a/lib/kadm5/rename_s.c +++ b/lib/kadm5/rename_s.c @@ -46,7 +46,7 @@ kadm5_s_rename_principal(void *server_handle, krb5_principal oldname; memset(&ent, 0, sizeof(ent)); - if(krb5_principal_compare(context->context, source, target)) + if (krb5_principal_compare(context->context, source, target)) return KADM5_DUP; /* XXX is this right? */ if (!context->keep_open) { ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0); @@ -61,11 +61,11 @@ kadm5_s_rename_principal(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, source, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); if (ret) - goto out; + goto out2; oldname = ent.entry.principal; ret = _kadm5_set_modifier(context, &ent.entry); if (ret) - goto out2; + goto out3; { /* fix salt */ size_t i; @@ -79,33 +79,35 @@ kadm5_s_rename_principal(void *server_handle, if(ent.entry.keys.val[i].salt == NULL){ ent.entry.keys.val[i].salt = malloc(sizeof(*ent.entry.keys.val[i].salt)); - if(ent.entry.keys.val[i].salt == NULL) - return ENOMEM; - ret = copy_Salt(&salt, ent.entry.keys.val[i].salt); - if(ret) + if (ent.entry.keys.val[i].salt == NULL) + ret = ENOMEM; + else + ret = copy_Salt(&salt, ent.entry.keys.val[i].salt); + if (ret) break; } } krb5_free_salt(context->context, salt2); } if (ret) - goto out2; + goto out3; /* Borrow target */ ent.entry.principal = target; ret = hdb_seal_keys(context->context, context->db, &ent.entry); if (ret) - goto out2; + goto out3; /* This logs the change for iprop and writes to the HDB */ ret = kadm5_log_rename(context, source, &ent.entry); -out2: + out3: ent.entry.principal = oldname; /* Unborrow target */ hdb_free_entry(context->context, &ent); -out: + out2: (void) kadm5_log_end(context); + out: if (!context->keep_open) { kadm5_ret_t ret2; ret2 = context->db->hdb_close(context->context, context->db); diff --git a/lib/kadm5/setkey3_s.c b/lib/kadm5/setkey3_s.c index 4776b814a..03b5d8101 100644 --- a/lib/kadm5/setkey3_s.c +++ b/lib/kadm5/setkey3_s.c @@ -33,8 +33,6 @@ #include "kadm5_locl.h" -RCSID("$Id$"); - /** * Server-side function to set new keys for a principal. */ @@ -65,10 +63,16 @@ kadm5_s_setkey_principal_3(void *server_handle, ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent); + if (ret) { + (void) kadm5_log_end(context); + if (!context->keep_open) + context->db->hdb_close(context->context, context->db); + return ret; + } + if (keepold) { - if (ret == 0) - ret = hdb_add_current_keys_to_history(context->context, &ent.entry); - } else if (ret == 0) + ret = hdb_add_current_keys_to_history(context->context, &ent.entry); + } else ret = hdb_clear_extension(context->context, &ent.entry, choice_HDB_extension_data_hist_keys); -- 2.11.4.GIT