From c58b50e0282deff6be81c74b2587d3331c6ccbc9 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 4 Oct 2021 16:21:51 +1300 Subject: [PATCH] CVE-2020-25719 kdc: Always confirm with HDB that a particular PKINIT principal is valid This allows us to have HDB check the principal specifically, distinct from the lookup at the top of the function. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14785 Signed-off-by: Andrew Bartlett --- source4/heimdal/kdc/pkinit.c | 22 +++++++++++++--- source4/heimdal/lib/hdb/hdb.h | 2 +- source4/kdc/db-glue.c | 49 ++++++++++++++++++++++++++--------- source4/kdc/sdb.h | 5 ++++ 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/source4/heimdal/kdc/pkinit.c b/source4/heimdal/kdc/pkinit.c index ad7f3efc10a..806d105f954 100644 --- a/source4/heimdal/kdc/pkinit.c +++ b/source4/heimdal/kdc/pkinit.c @@ -1575,12 +1575,15 @@ match_rfc_san(krb5_context context, krb5_kdc_configuration *config, hx509_context hx509ctx, hx509_cert client_cert, - krb5_const_principal match) + HDB *clientdb, + hdb_entry_ex *client) { hx509_octet_string_list list; int ret, found = 0; size_t i; + krb5_const_principal match = client->entry.principal; + memset(&list, 0 , sizeof(list)); ret = hx509_cert_find_subjectAltName_otherName(hx509ctx, @@ -1614,8 +1617,18 @@ match_rfc_san(krb5_context context, principal.name = kn.principalName; principal.realm = kn.realm; - if (krb5_principal_compare(context, &principal, match) == TRUE) - found = 1; + if (clientdb->hdb_check_pkinit_ms_upn_match) { + ret = clientdb->hdb_check_pkinit_ms_upn_match(context, + clientdb, + client, + &principal); + if (ret == 0) { + found = 1; + } + } else { + if (krb5_principal_compare(context, &principal, match) == TRUE) + found = 1; + } free_KRB5PrincipalName(&kn); } @@ -1765,7 +1778,8 @@ _kdc_pk_check_client(krb5_context context, ret = match_rfc_san(context, config, context->hx509ctx, cp->cert, - client->entry.principal); + clientdb, + client); if (ret == 0) { kdc_log(context, config, 5, "Found matching PK-INIT SAN in certificate"); diff --git a/source4/heimdal/lib/hdb/hdb.h b/source4/heimdal/lib/hdb/hdb.h index 5ef9d9565f3..ecfe9163fea 100644 --- a/source4/heimdal/lib/hdb/hdb.h +++ b/source4/heimdal/lib/hdb/hdb.h @@ -259,7 +259,7 @@ typedef struct HDB{ krb5_error_code (*hdb_check_constrained_delegation)(krb5_context, struct HDB *, hdb_entry_ex *, krb5_const_principal); /** - * Check if this name is an alias for the supplied client for PKINIT userPrinicpalName logins + * Check if this name is a valid name or alias for the supplied client for PKINIT userPrinicpalName logins */ krb5_error_code (*hdb_check_pkinit_ms_upn_match)(krb5_context, struct HDB *, hdb_entry_ex *, krb5_const_principal); diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index bc395ee534b..ffd482ea44c 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -1663,15 +1663,17 @@ static krb5_error_code samba_kdc_lookup_trust(krb5_context context, struct ldb_c } static krb5_error_code samba_kdc_lookup_client(krb5_context context, - struct samba_kdc_db_context *kdc_db_ctx, - TALLOC_CTX *mem_ctx, - krb5_const_principal principal, - const char **attrs, - struct ldb_dn **realm_dn, - struct ldb_message **msg) + struct samba_kdc_db_context *kdc_db_ctx, + TALLOC_CTX *mem_ctx, + krb5_const_principal principal, + unsigned flags, + const char **attrs, + struct ldb_dn **realm_dn, + struct ldb_message **msg) { NTSTATUS nt_status; char *principal_string = NULL; + bool is_pkinit; if (smb_krb5_principal_get_type(context, principal) == KRB5_NT_ENTERPRISE_PRINCIPAL) { principal_string = smb_krb5_principal_get_comp_string(mem_ctx, context, @@ -1698,7 +1700,13 @@ static krb5_error_code samba_kdc_lookup_client(krb5_context context, nt_status = sam_get_results_principal(kdc_db_ctx->samdb, mem_ctx, principal_string, attrs, realm_dn, msg); - if (NT_STATUS_EQUAL(nt_status, NT_STATUS_NO_SUCH_USER)) { + /* + * The name inside a certificate must match the + * userPrincipalName or samAccountName implicit UPN exactly, + * we don't want this fallback for certificates + */ + is_pkinit = flags & SDB_F_GET_FOR_PKINIT; + if (NT_STATUS_EQUAL(nt_status, NT_STATUS_NO_SUCH_USER) && !is_pkinit) { krb5_principal fallback_principal = NULL; unsigned int num_comp; char *fallback_realm = NULL; @@ -1793,7 +1801,9 @@ static krb5_error_code samba_kdc_fetch_client(krb5_context context, struct ldb_message *msg = NULL; ret = samba_kdc_lookup_client(context, kdc_db_ctx, - mem_ctx, principal, user_attrs, + mem_ctx, principal, + flags, + user_attrs, &realm_dn, &msg); if (ret != 0) { return ret; @@ -2006,7 +2016,8 @@ static krb5_error_code samba_kdc_lookup_server(krb5_context context, * not AS-REQ packets. */ return samba_kdc_lookup_client(context, kdc_db_ctx, - mem_ctx, principal, attrs, + mem_ctx, principal, flags, + attrs, realm_dn, msg); } else { /* @@ -2575,10 +2586,19 @@ samba_kdc_check_s4u2self(krb5_context context, return 0; } -/* Certificates printed by a the Certificate Authority might have a +/* + * Certificates printed by a the Certificate Authority might have a * slightly different form of the user principal name to that in the - * database. Allow a mismatch where they both refer to the same - * SID */ + * database. + * + * This will Allow a mismatch where they both refer to the same SID (as + * otherwise case-wise changes in the outer AS-REQ name would cause + * problems. + * + * However don't do the trailing$ expansion for a CA-issued + * certificate, that shouldn't happen and we can be stricter on this + * point. + */ krb5_error_code samba_kdc_check_pkinit_ms_upn_match(krb5_context context, @@ -2603,8 +2623,13 @@ samba_kdc_check_pkinit_ms_upn_match(krb5_context context, return ret; } + /* + * SDB_F_GET_FOR_PKINIT disables the trailing $ alias for this + * lookup + */ ret = samba_kdc_lookup_client(context, kdc_db_ctx, mem_ctx, certificate_principal, + SDB_F_GET_FOR_PKINIT, ms_upn_check_attrs, &realm_dn, &msg); if (ret != 0) { diff --git a/source4/kdc/sdb.h b/source4/kdc/sdb.h index c929acccce6..5eaf40082b1 100644 --- a/source4/kdc/sdb.h +++ b/source4/kdc/sdb.h @@ -117,6 +117,11 @@ struct sdb_entry_ex { #define SDB_F_FOR_AS_REQ 4096 /* fetch is for a AS REQ */ #define SDB_F_FOR_TGS_REQ 8192 /* fetch is for a TGS REQ */ +/* Samba-only flags, not in Heimdal currently */ + +/* This name came from PKINIT, use PKINIT lookup rules */ +#define SDB_F_GET_FOR_PKINIT 0x80000000 + void sdb_free_entry(struct sdb_entry_ex *e); void free_sdb_entry(struct sdb_entry *s); struct SDBFlags int2SDBFlags(unsigned n); -- 2.25.1