Attachment 17267 Details for Bug 14785 – patch (againt old master) for this issue

[patch] patch (againt old master) for this issue

0001-CVE-2020-25719-kdc-Always-confirm-with-HDB-that-a-pa.patch (text/plain), 7.03 KB, created by Andrew Bartlett on 2022-04-13 01:49:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2022-04-13 01:49:20 UTC

Size: 7.03 KB

patch

obsolete

>From c58b50e0282deff6be81c74b2587d3331c6ccbc9 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>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 <abartlet@samba.org>
>---
> 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
>

Actions: View

Attachments on bug 14785: 17267