From bb643664a28e4f371629e358bd99c2d6f6444446 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 24 May 2022 10:17:00 +0200 Subject: [PATCH 1/5] testprogs: Fix auth with smbclient and krb5 ccache BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047 Signed-off-by: Andreas Schneider --- testprogs/blackbox/test_kpasswd_heimdal.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testprogs/blackbox/test_kpasswd_heimdal.sh b/testprogs/blackbox/test_kpasswd_heimdal.sh index 43f38b09de2..a73c6665a18 100755 --- a/testprogs/blackbox/test_kpasswd_heimdal.sh +++ b/testprogs/blackbox/test_kpasswd_heimdal.sh @@ -71,7 +71,7 @@ testit "kinit with user password" \ do_kinit $TEST_PRINCIPAL $TEST_PASSWORD || failed=`expr $failed + 1` test_smbclient "Test login with user kerberos ccache" \ - "ls" "$SMB_UNC" --use-kerberos=required || failed=`expr $failed + 1` + "ls" "$SMB_UNC" --use-krb5-ccache=${KRB5CCNAME} || failed=`expr $failed + 1` testit "change user password with 'samba-tool user password' (unforced)" \ $VALGRIND $PYTHON $samba_tool user password -W$DOMAIN -U$TEST_USERNAME%$TEST_PASSWORD --use-kerberos=off --newpassword=$TEST_PASSWORD_NEW || failed=`expr $failed + 1` @@ -84,7 +84,7 @@ testit "kinit with user password" \ do_kinit $TEST_PRINCIPAL $TEST_PASSWORD || failed=`expr $failed + 1` test_smbclient "Test login with user kerberos ccache" \ - "ls" "$SMB_UNC" --use-kerberos=required || failed=`expr $failed + 1` + "ls" "$SMB_UNC" --use-krb5-ccache=${KRB5CCNAME} || failed=`expr $failed + 1` ########################################################### ### check that a short password is rejected -- 2.36.1 From 24919c0c10551c6f151b8cf59882c886239313ca Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 19 May 2022 16:35:28 +0200 Subject: [PATCH 2/5] testprogs: Add test with MIT kpasswd for CVE-2022-XXXXX BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047 Signed-off-by: Andreas Schneider --- selftest/knownfail.d/kadmin_changew | 1 + testprogs/blackbox/test_kpasswd_heimdal.sh | 35 +++++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/kadmin_changew diff --git a/selftest/knownfail.d/kadmin_changew b/selftest/knownfail.d/kadmin_changew new file mode 100644 index 00000000000..97c14793ea5 --- /dev/null +++ b/selftest/knownfail.d/kadmin_changew @@ -0,0 +1 @@ +^samba4.blackbox.kpasswd.MIT kpasswd.change.user.password diff --git a/testprogs/blackbox/test_kpasswd_heimdal.sh b/testprogs/blackbox/test_kpasswd_heimdal.sh index a73c6665a18..698044a3fd3 100755 --- a/testprogs/blackbox/test_kpasswd_heimdal.sh +++ b/testprogs/blackbox/test_kpasswd_heimdal.sh @@ -7,7 +7,7 @@ if [ $# -lt 6 ]; then cat < "${PREFIX}/tmpkpasswdscript" < "${KRB5_CONFIG}" + testit "MIT kpasswd change user password" \ + "${texpect}" "${PREFIX}/tmpkpasswdscript" "${mit_kpasswd}" \ + "${TEST_PRINCIPAL}" || + failed=$((failed + 1)) + KRB5_CONFIG="${SAVE_KRB5_CONFIG}" + export KRB5_CONFIG +fi + +TEST_PASSWORD="${TEST_PASSWORD_NEW}" +TEST_PASSWORD_NEW="testPaSS@03force%" + ########################################################### ### Force password change at login ########################################################### -- 2.36.1 From 96c4e4bcc3a1f249642b89637241d116a6a7958a Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 24 May 2022 09:54:18 +0200 Subject: [PATCH 3/5] s4:kdc: Implement is_kadmin_changepw() helper function BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047 Signed-off-by: Andreas Schneider --- source4/kdc/db-glue.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index ea329b7edab..b03a32b67f5 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -917,6 +917,14 @@ static int principal_comp_strcmp(krb5_context context, component, string, false); } +static bool is_kadmin_changepw(krb5_context context, + krb5_const_principal principal) +{ + return krb5_princ_size(context, principal) == 2 && + (principal_comp_strcmp(context, principal, 0, "kadmin") == 0) && + (principal_comp_strcmp(context, principal, 1, "changepw") == 0); +} + /* * Construct an hdb_entry from a directory entry. */ @@ -1221,11 +1229,9 @@ static krb5_error_code samba_kdc_message2entry(krb5_context context, * 'change password', as otherwise we could get into * trouble, and not enforce the password expirty. * Instead, only do it when request is for the kpasswd service */ - if (ent_type == SAMBA_KDC_ENT_TYPE_SERVER - && krb5_princ_size(context, principal) == 2 - && (principal_comp_strcmp(context, principal, 0, "kadmin") == 0) - && (principal_comp_strcmp(context, principal, 1, "changepw") == 0) - && lpcfg_is_my_domain_or_realm(lp_ctx, realm)) { + if (ent_type == SAMBA_KDC_ENT_TYPE_SERVER && + is_kadmin_changepw(context, principal) && + lpcfg_is_my_domain_or_realm(lp_ctx, realm)) { entry->flags.change_pw = 1; } -- 2.36.1 From 334d95a153b184fcdd15509e2880d8ebceb1fbe7 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 18 May 2022 16:56:01 +1200 Subject: [PATCH 4/5] s4:kdc: Split out a samba_kdc_get_entry_principal() function BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider --- source4/kdc/db-glue.c | 169 ++++++++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 73 deletions(-) diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index b03a32b67f5..5a71a4b281f 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -925,6 +925,90 @@ static bool is_kadmin_changepw(krb5_context context, (principal_comp_strcmp(context, principal, 1, "changepw") == 0); } +static krb5_error_code samba_kdc_get_entry_principal( + krb5_context context, + struct samba_kdc_db_context *kdc_db_ctx, + const char *samAccountName, + enum samba_kdc_ent_type ent_type, + unsigned flags, + krb5_const_principal in_princ, + krb5_principal *out_princ) +{ + struct loadparm_context *lp_ctx = kdc_db_ctx->lp_ctx; + krb5_error_code code = 0; + + if (ent_type == SAMBA_KDC_ENT_TYPE_ANY && + in_princ == NULL) { + code = smb_krb5_make_principal(context, + out_princ, + lpcfg_realm(lp_ctx), + samAccountName, NULL); + return code; + } + + if (flags & (SDB_F_CANON|SDB_F_FORCE_CANON)) { + if (ent_type == SAMBA_KDC_ENT_TYPE_KRBTGT) { + /* + * When requested to do so, ensure that the + * both realm values in the principal are set + * to the upper case, canonical realm + */ + code = smb_krb5_make_principal(context, + out_princ, + lpcfg_realm(lp_ctx), + "krbtgt", + lpcfg_realm(lp_ctx), + NULL); + if (code != 0) { + return code; + } + smb_krb5_principal_set_type(context, + *out_princ, + KRB5_NT_SRV_INST); + + return 0; + } + + if (flags & (SDB_F_FORCE_CANON|SDB_F_FOR_AS_REQ)) { + /* + * SDB_F_CANON maps from the canonicalize flag in the + * packet, and has a different meaning between AS-REQ + * and TGS-REQ. We only change the principal in the + * AS-REQ case. + * + * The SDB_F_FORCE_CANON if for new MIT KDC code that + * wants the canonical name in all lookups, and takes + * care to canonicalize only when appropriate. + */ + code = smb_krb5_make_principal(context, + out_princ, + lpcfg_realm(lp_ctx), + samAccountName, + NULL); + return code; + } + } + + /* + * For a krbtgt entry, this appears to be required regardless of the + * canonicalize flag from the client. + */ + code = krb5_copy_principal(context, in_princ, out_princ); + if (code != 0) { + return code; + } + + /* + * While we have copied the client principal, tests show that Win2k3 + * returns the 'corrected' realm, not the client-specified realm. This + * code attempts to replace the client principal's realm with the one + * we determine from our records + */ + code = smb_krb5_principal_set_realm(context, *out_princ, lpcfg_realm(lp_ctx)); + + return code; +} + /* * Construct an hdb_entry from a directory entry. */ @@ -1031,79 +1115,6 @@ static krb5_error_code samba_kdc_message2entry(krb5_context context, if (ent_type == SAMBA_KDC_ENT_TYPE_KRBTGT) { p->is_krbtgt = true; - - if (flags & (SDB_F_CANON|SDB_F_FORCE_CANON)) { - /* - * When requested to do so, ensure that the - * both realm values in the principal are set - * to the upper case, canonical realm - */ - ret = smb_krb5_make_principal(context, &entry->principal, - lpcfg_realm(lp_ctx), "krbtgt", - lpcfg_realm(lp_ctx), NULL); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - smb_krb5_principal_set_type(context, entry->principal, KRB5_NT_SRV_INST); - } else { - ret = krb5_copy_principal(context, principal, &entry->principal); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - /* - * this appears to be required regardless of - * the canonicalize flag from the client - */ - ret = smb_krb5_principal_set_realm(context, entry->principal, lpcfg_realm(lp_ctx)); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - } - - } else if (ent_type == SAMBA_KDC_ENT_TYPE_ANY && principal == NULL) { - ret = smb_krb5_make_principal(context, &entry->principal, lpcfg_realm(lp_ctx), samAccountName, NULL); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - } else if ((flags & SDB_F_FORCE_CANON) || - ((flags & SDB_F_CANON) && (flags & SDB_F_FOR_AS_REQ))) { - /* - * SDB_F_CANON maps from the canonicalize flag in the - * packet, and has a different meaning between AS-REQ - * and TGS-REQ. We only change the principal in the AS-REQ case - * - * The SDB_F_FORCE_CANON if for new MIT KDC code that wants - * the canonical name in all lookups, and takes care to - * canonicalize only when appropriate. - */ - ret = smb_krb5_make_principal(context, &entry->principal, lpcfg_realm(lp_ctx), samAccountName, NULL); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - } else { - ret = krb5_copy_principal(context, principal, &entry->principal); - if (ret) { - krb5_clear_error_message(context); - goto out; - } - - /* While we have copied the client principal, tests - * show that Win2k3 returns the 'corrected' realm, not - * the client-specified realm. This code attempts to - * replace the client principal's realm with the one - * we determine from our records */ - - /* this has to be with malloc() */ - ret = smb_krb5_principal_set_realm(context, entry->principal, lpcfg_realm(lp_ctx)); - if (ret) { - krb5_clear_error_message(context); - goto out; - } } /* First try and figure out the flags based on the userAccountControl */ @@ -1296,6 +1307,18 @@ static krb5_error_code samba_kdc_message2entry(krb5_context context, } } + ret = samba_kdc_get_entry_principal(context, + kdc_db_ctx, + samAccountName, + ent_type, + flags, + principal, + &entry->principal); + if (ret != 0) { + krb5_clear_error_message(context); + goto out; + } + entry->valid_start = NULL; entry->max_life = malloc(sizeof(*entry->max_life)); -- 2.36.1 From 1ae9cd5892194b54d22a66f3409540e4feca417a Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 18 May 2022 16:56:01 +1200 Subject: [PATCH 5/5] CVE-2022-XXXXXX: s4:kdc: Fix canonicalisation of kadmin/changepw principal Since this principal goes through the samba_kdc_fetch_server() path, setting the canonicalisation flag would cause the principal to be replaced with the sAMAccountName; this meant requests to kadmin/changepw@REALM would result in a ticket to krbtgt@REALM. Now we properly handle canonicalisation for the kadmin/changepw principal. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047 Pair-Programmed-With: Andreas Schneider Signed-off-by: Andreas Schneider Signed-off-by: Joseph Sutton --- selftest/knownfail.d/kadmin_changew | 1 - source4/kdc/db-glue.c | 11 +++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) delete mode 100644 selftest/knownfail.d/kadmin_changew diff --git a/selftest/knownfail.d/kadmin_changew b/selftest/knownfail.d/kadmin_changew deleted file mode 100644 index 97c14793ea5..00000000000 --- a/selftest/knownfail.d/kadmin_changew +++ /dev/null @@ -1 +0,0 @@ -^samba4.blackbox.kpasswd.MIT kpasswd.change.user.password diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index 5a71a4b281f..c2082e5ec94 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -931,14 +931,20 @@ static krb5_error_code samba_kdc_get_entry_principal( const char *samAccountName, enum samba_kdc_ent_type ent_type, unsigned flags, + bool is_kadmin_changepw, krb5_const_principal in_princ, krb5_principal *out_princ) { struct loadparm_context *lp_ctx = kdc_db_ctx->lp_ctx; krb5_error_code code = 0; + /* + * We need to ensure that the kadmin/changepw princial it isn't able to + * get krbtgt ticket, even if canonicalization is turned on. + */ if (ent_type == SAMBA_KDC_ENT_TYPE_ANY && - in_princ == NULL) { + in_princ == NULL && + !is_kadmin_changepw) { code = smb_krb5_make_principal(context, out_princ, lpcfg_realm(lp_ctx), @@ -946,7 +952,7 @@ static krb5_error_code samba_kdc_get_entry_principal( return code; } - if (flags & (SDB_F_CANON|SDB_F_FORCE_CANON)) { + if ((flags & (SDB_F_CANON|SDB_F_FORCE_CANON)) && !is_kadmin_changepw) { if (ent_type == SAMBA_KDC_ENT_TYPE_KRBTGT) { /* * When requested to do so, ensure that the @@ -1312,6 +1318,7 @@ static krb5_error_code samba_kdc_message2entry(krb5_context context, samAccountName, ent_type, flags, + entry->flags.change_pw, principal, &entry->principal); if (ret != 0) { -- 2.36.1