From c89d3f3b8c1a908c7bb4573f93258d2d469a7fe8 Mon Sep 17 00:00:00 2001 From: Luke Howard Date: Mon, 24 Dec 2018 15:28:32 +1100 Subject: [PATCH] kadmin: allow enforcing password quality on admin password change This patch adds the "enforce_on_admin_set" configuration knob in the [password_quality] section. When this is enabled, administrative password changes via the kadmin or kpasswd protocols will be subject to password quality checks. (An administrative password change is one where the authenticating principal is different to the principal whose password is being changed.) Note that kadmin running in local mode (-l) is unaffected by this patch. --- doc/setup.texi | 9 +++++ kadmin/server.c | 76 +++++++++++++++++++++++++++---------- kpasswd/kpasswdd.c | 40 +++++++++++-------- lib/krb5/verify_krb5_conf.c | 1 + tests/kdc/check-kadmin.in | 7 +++- tests/ldap/check-ldap.in | 10 +++-- 6 files changed, 103 insertions(+), 40 deletions(-) diff --git a/doc/setup.texi b/doc/setup.texi index d354049bc..f0e298784 100644 --- a/doc/setup.texi +++ b/doc/setup.texi @@ -470,6 +470,15 @@ classes. Default value if not given is 3. The four different characters classes are, uppercase, lowercase, number, special characters. +@item enforce_on_admin_set + +The enforce_on_admin_set check validates that administrative password changes +via kpasswdd or kadmind are also subject to the password policy. Note that +@command{kadmin} in local mode can still bypass these. An administrative +password change is one where the identity of the authenticating principal +differs from the subject of the password change. Default value if not given is +true. + @end itemize If you want to write your own shared object to check password diff --git a/kadmin/server.c b/kadmin/server.c index ccb6a7a99..03605e504 100644 --- a/kadmin/server.c +++ b/kadmin/server.c @@ -38,6 +38,9 @@ static kadm5_ret_t check_aliases(kadm5_server_context *, kadm5_principal_ent_rec *, kadm5_principal_ent_rec *); +static krb5_boolean +enforce_pwqual_on_admin_set_p(kadm5_server_context *contextp); + static kadm5_ret_t kadmind_dispatch(void *kadm_handlep, krb5_boolean initial, krb5_data *in, krb5_data *out) @@ -178,6 +181,24 @@ kadmind_dispatch(void *kadm_handlep, krb5_boolean initial, } krb5_unparse_name_fixed(contextp->context, ent.principal, name, sizeof(name)); + if (enforce_pwqual_on_admin_set_p(contextp)) { + krb5_data pwd_data; + const char *pwd_reason; + + pwd_data.data = password; + pwd_data.length = strlen(password); + + pwd_reason = kadm5_check_password_quality (contextp->context, + ent.principal, &pwd_data); + if (pwd_reason != NULL) + ret = KADM5_PASS_Q_DICT; + else + ret = 0; + if (ret) { + kadm5_free_principal_ent(kadm_handlep, &ent); + goto fail; + } + } krb5_warnx(contextp->context, "%s: %s %s", client, op, name); ret = _kadm5_acl_check_permission(contextp, KADM5_PRIV_ADD, ent.principal); @@ -296,6 +317,8 @@ kadmind_dispatch(void *kadm_handlep, krb5_boolean initial, break; } case kadm_chpass:{ + krb5_boolean is_self_cpw, allow_self_cpw; + op = "CHPASS"; ret = krb5_ret_principal(sp, &princ); if (ret) @@ -314,21 +337,29 @@ kadmind_dispatch(void *kadm_handlep, krb5_boolean initial, krb5_warnx(contextp->context, "%s: %s %s", client, op, name); /* - * The change is allowed if at least one of: - * - * a) allowed by sysadmin - * b) it's for the principal him/herself and this was an - * initial ticket, but then, check with the password quality - * function. - * c) the user is on the CPW ACL. + * Change password requests are subject to ACLs unless the principal is + * changing their own password and the initial ticket flag is set, and + * the allow_self_change_password configuration option is TRUE. */ + is_self_cpw = + krb5_principal_compare(contextp->context, contextp->caller, princ); + allow_self_cpw = + krb5_config_get_bool_default(contextp->context, NULL, TRUE, + "kadmin", "allow_self_change_password", NULL); + if (!(is_self_cpw && initial && allow_self_cpw)) { + ret = _kadm5_acl_check_permission(contextp, KADM5_PRIV_CPW, princ); + if (ret) { + krb5_free_principal(contextp->context, princ); + goto fail; + } + } - if (krb5_config_get_bool_default(contextp->context, NULL, TRUE, - "kadmin", "allow_self_change_password", NULL) - && initial - && krb5_principal_compare (contextp->context, contextp->caller, - princ)) - { + /* + * Change password requests are subject to password quality checks if + * the principal is changing their own password, or the enforce_on_admin_set + * configuration option is TRUE (the default). + */ + if (is_self_cpw || enforce_pwqual_on_admin_set_p(contextp)) { krb5_data pwd_data; const char *pwd_reason; @@ -341,13 +372,12 @@ kadmind_dispatch(void *kadm_handlep, krb5_boolean initial, ret = KADM5_PASS_Q_DICT; else ret = 0; - } else - ret = _kadm5_acl_check_permission(contextp, KADM5_PRIV_CPW, princ); - - if(ret) { - krb5_free_principal(contextp->context, princ); - goto fail; + if (ret) { + krb5_free_principal(contextp->context, princ); + goto fail; + } } + ret = kadm5_chpass_principal_3(kadm_handlep, princ, keepold, 0, NULL, password); krb5_free_principal(contextp->context, princ); @@ -843,3 +873,11 @@ kadmind_loop(krb5_context contextp, return 0; } + +static krb5_boolean +enforce_pwqual_on_admin_set_p(kadm5_server_context *contextp) +{ + return krb5_config_get_bool_default(contextp->context, NULL, TRUE, + "password_quality", + "enforce_on_admin_set", NULL); +} diff --git a/kpasswd/kpasswdd.c b/kpasswd/kpasswdd.c index 967bf9383..cc115ce18 100644 --- a/kpasswd/kpasswdd.c +++ b/kpasswd/kpasswdd.c @@ -253,6 +253,7 @@ change (krb5_auth_context auth_context, krb5_data *pwd_data = NULL; char *tmp; ChangePasswdDataMS chpw; + krb5_boolean enforce_pwqual_on_admin_set; memset (&conf, 0, sizeof(conf)); memset(&chpw, 0, sizeof(chpw)); @@ -366,12 +367,31 @@ change (krb5_auth_context auth_context, goto out; } + if (krb5_principal_compare(context, admin_principal, principal) == FALSE) { + ret = _kadm5_acl_check_permission(kadm5_handle, KADM5_PRIV_CPW, + principal); + if (ret) { + krb5_warn (context, ret, + "Check ACL failed for %s for changing %s password", + admin, client); + reply_priv (auth_context, s, sa, sa_size, + KRB5_KPASSWD_HARDERROR, "permission denied"); + goto out; + } + krb5_warnx (context, "%s is changing password for %s", admin, client); + } + /* - * Check password quality if not changing as administrator + * Change password requests are subject to password quality checks if + * the principal is changing their own password, or the enforce_on_admin_set + * configuration option is TRUE (the default). */ - - if (krb5_principal_compare(context, admin_principal, principal) == TRUE) { - + enforce_pwqual_on_admin_set = + krb5_config_get_bool_default(context, NULL, TRUE, + "password_quality", + "enforce_on_admin_set", NULL); + if (krb5_principal_compare(context, admin_principal, principal) == TRUE || + enforce_pwqual_on_admin_set == TRUE) { pwd_reason = kadm5_check_password_quality (context, principal, pwd_data); if (pwd_reason != NULL ) { @@ -383,18 +403,6 @@ change (krb5_auth_context auth_context, goto out; } krb5_warnx (context, "Changing password for %s", client); - } else { - ret = _kadm5_acl_check_permission(kadm5_handle, KADM5_PRIV_CPW, - principal); - if (ret) { - krb5_warn (context, ret, - "Check ACL failed for %s for changing %s password", - admin, client); - reply_priv (auth_context, s, sa, sa_size, - KRB5_KPASSWD_HARDERROR, "permission denied"); - goto out; - } - krb5_warnx (context, "%s is changing password for %s", admin, client); } ret = krb5_data_realloc(pwd_data, pwd_data->length + 1); diff --git a/lib/krb5/verify_krb5_conf.c b/lib/krb5/verify_krb5_conf.c index 7528f3f7b..232f2db18 100644 --- a/lib/krb5/verify_krb5_conf.c +++ b/lib/krb5/verify_krb5_conf.c @@ -633,6 +633,7 @@ struct entry kcm_entries[] = { }; struct entry password_quality_entries[] = { + { "enforce_on_admin_set", krb5_config_string, check_boolean, 0 }, { "check_function", krb5_config_string, NULL, 0 }, { "check_library", krb5_config_string, NULL, 0 }, { "external_program", krb5_config_string, NULL, 0 }, diff --git a/tests/kdc/check-kadmin.in b/tests/kdc/check-kadmin.in index 527231900..106ef70c9 100644 --- a/tests/kdc/check-kadmin.in +++ b/tests/kdc/check-kadmin.in @@ -59,7 +59,7 @@ kinit="${kinit} -c $cache ${afs_no_afslog}" kgetcred="${kgetcred} -c $cache" kdestroy="${kdestroy} -c $cache ${afs_no_unlog}" -foopassword="foo" +foopassword="fooLongPasswordYo123;" KRB5_CONFIG="${objdir}/krb5.conf" export KRB5_CONFIG @@ -256,6 +256,11 @@ env KRB5CCNAME=${cache} \ ${kadmin} -p foo/admin@${R} add -p "$foopassword" --use-defaults kaka@${R} || { echo "kadmin failed $?"; cat messages.log ; exit 1; } +echo "kadmin" +env KRB5CCNAME=${cache} \ +${kadmin} -p foo/admin@${R} add -p abc --use-defaults kaka@${R} && + { echo "kadmin succeeded $?"; cat messages.log ; exit 1; } + #---------------------------------- ${kadmind} -d & kadmpid=$! diff --git a/tests/ldap/check-ldap.in b/tests/ldap/check-ldap.in index 9d6d7f305..b99c95103 100644 --- a/tests/ldap/check-ldap.in +++ b/tests/ldap/check-ldap.in @@ -54,6 +54,8 @@ kgetcred="${TESTS_ENVIRONMENT} ../../kuser/kgetcred -c $cache" kadmin="${TESTS_ENVIRONMENT} ../../kadmin/kadmin -l -r $R" kdc="${TESTS_ENVIRONMENT} ../../kdc/kdc --addresses=localhost -P $port" +foopassword="fooLongPasswordYo123;" + testfailed="echo test failed; exit 1" KRB5_CONFIG="${objdir}/krb5.conf" @@ -102,8 +104,8 @@ ${kadmin} \ --realm-max-renewable-life=1month \ ${R} || exit 1 -${kadmin} add -p foo --use-defaults foo@${R} || exit 1 -${kadmin} add -p foo --use-defaults bar@${R} || exit 1 +${kadmin} add -p "$foopassword" --use-defaults foo@${R} || exit 1 +${kadmin} add -p "$foopassword" --use-defaults bar@${R} || exit 1 ${kadmin} add -p kaka --use-defaults ${server}@${R} || exit 1 ${kadmin} cpw --random-password bar@${R} > /dev/null || exit 1 @@ -111,11 +113,11 @@ ${kadmin} cpw --random-password bar@${R} > /dev/null || exit 1 ${kadmin} cpw --random-password bar@${R} > /dev/null || exit 1 ${kadmin} cpw --random-password suser@${R} > /dev/null|| exit 1 -${kadmin} cpw --password=foo suser@${R} || exit 1 +${kadmin} cpw --password="$foopassword" suser@${R} || exit 1 ${kadmin} list '*' > /dev/null || exit 1 -echo foo > ${objdir}/foopassword +echo "$foopassword" > ${objdir}/foopassword echo Starting kdc ${kdc} --detach --testing || { echo "kdc failed to start"; exit 1; }