From 8d94ec0d3f7d3271aa9499e6a9c535ba2efc8f57 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 8 Oct 2021 08:29:51 +1300 Subject: [PATCH] CVE-2020-25719 kdc: Avoid races and multiple DB lookups in s4u2self check Looking up the DB twice is subject to a race and is a poor use of resources, so instead just pass in the record we already got when trying to confirm that the server in S4U2Self is the same as the requesting client. The client record has already been bound to the the original client by the SID check in the PAC. Likewise by looking up server only once we ensure that the keys looked up originally are in the record we confirm the SID for here. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14686 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton --- source4/heimdal/kdc/krb5tgs.c | 26 +++++++++++------ source4/heimdal/lib/hdb/hdb.h | 2 +- source4/kdc/db-glue.c | 54 ++++++++++++----------------------- source4/kdc/db-glue.h | 5 ++-- source4/kdc/hdb-samba4.c | 43 ++++++++-------------------- 5 files changed, 52 insertions(+), 78 deletions(-) diff --git a/source4/heimdal/kdc/krb5tgs.c b/source4/heimdal/kdc/krb5tgs.c index 301ca92091a..d4a1c78e153 100644 --- a/source4/heimdal/kdc/krb5tgs.c +++ b/source4/heimdal/kdc/krb5tgs.c @@ -313,7 +313,7 @@ check_constrained_delegation(krb5_context context, * Determine if s4u2self is allowed from this client to this server * * For example, regardless of the principal being impersonated, if the - * 'client' and 'server' are the same, then it's safe. + * 'client' and 'server' (target) are the same, then it's safe. */ static krb5_error_code @@ -321,18 +321,28 @@ check_s4u2self(krb5_context context, krb5_kdc_configuration *config, HDB *clientdb, hdb_entry_ex *client, - krb5_const_principal server) + hdb_entry_ex *target_server, + krb5_const_principal target_server_principal) { krb5_error_code ret; - /* if client does a s4u2self to itself, that ok */ - if (krb5_principal_compare(context, client->entry.principal, server) == TRUE) - return 0; - + /* + * Always allow the plugin to check, this might be faster, allow a + * policy or audit check and can look into the DB records + * directly + */ if (clientdb->hdb_check_s4u2self) { - ret = clientdb->hdb_check_s4u2self(context, clientdb, client, server); + ret = clientdb->hdb_check_s4u2self(context, + clientdb, + client, + target_server); if (ret == 0) return 0; + } else if (krb5_principal_compare(context, + client->entry.principal, + target_server_principal) == TRUE) { + /* if client does a s4u2self to itself, and there is no plugin, that is ok */ + return 0; } else { ret = KRB5KDC_ERR_BADOPTION; } @@ -1774,7 +1784,7 @@ server_lookup: * Check that service doing the impersonating is * requesting a ticket to it-self. */ - ret = check_s4u2self(context, config, clientdb, client, sp); + ret = check_s4u2self(context, config, clientdb, client, server, sp); if (ret) { kdc_log(context, config, 0, "S4U2Self: %s is not allowed " "to impersonate to service " diff --git a/source4/heimdal/lib/hdb/hdb.h b/source4/heimdal/lib/hdb/hdb.h index 6a09ecb6fe1..5ef9d9565f3 100644 --- a/source4/heimdal/lib/hdb/hdb.h +++ b/source4/heimdal/lib/hdb/hdb.h @@ -266,7 +266,7 @@ typedef struct HDB{ /** * Check if s4u2self is allowed from this client to this server */ - krb5_error_code (*hdb_check_s4u2self)(krb5_context, struct HDB *, hdb_entry_ex *, krb5_const_principal); + krb5_error_code (*hdb_check_s4u2self)(krb5_context, struct HDB *, hdb_entry_ex *, hdb_entry_ex *); }HDB; #define HDB_INTERFACE_VERSION 7 diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index 5fd0f431cdf..d55bf1663d4 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -2508,53 +2508,37 @@ krb5_error_code samba_kdc_nextkey(krb5_context context, /* Check if a given entry may delegate or do s4u2self to this target principal * - * This is currently a very nasty hack - allowing only delegation to itself. + * The safest way to determine 'self' is to check the DB record made at + * the time the principal was presented to the KDC. */ krb5_error_code samba_kdc_check_s4u2self(krb5_context context, - struct samba_kdc_db_context *kdc_db_ctx, - struct samba_kdc_entry *skdc_entry, - krb5_const_principal target_principal) + struct samba_kdc_entry *skdc_entry_client, + struct samba_kdc_entry *skdc_entry_server_target) { - krb5_error_code ret; - struct ldb_dn *realm_dn; - struct ldb_message *msg; struct dom_sid *orig_sid; struct dom_sid *target_sid; - const char *delegation_check_attrs[] = { - "objectSid", NULL - }; - - TALLOC_CTX *mem_ctx = talloc_named(kdc_db_ctx, 0, "samba_kdc_check_s4u2self"); - - if (!mem_ctx) { - ret = ENOMEM; - krb5_set_error_message(context, ret, "samba_kdc_check_s4u2self: talloc_named() failed!"); - return ret; - } - - ret = samba_kdc_lookup_server(context, kdc_db_ctx, mem_ctx, target_principal, - SDB_F_GET_CLIENT|SDB_F_GET_SERVER, - delegation_check_attrs, &realm_dn, &msg); - - if (ret != 0) { - talloc_free(mem_ctx); - return ret; - } + TALLOC_CTX *frame = talloc_stackframe(); - orig_sid = samdb_result_dom_sid(mem_ctx, skdc_entry->msg, "objectSid"); - target_sid = samdb_result_dom_sid(mem_ctx, msg, "objectSid"); + orig_sid = samdb_result_dom_sid(frame, + skdc_entry_client->msg, + "objectSid"); + target_sid = samdb_result_dom_sid(frame, + skdc_entry_server_target->msg, + "objectSid"); - /* Allow delegation to the same principal, even if by a different - * name. The easy and safe way to prove this is by SID - * comparison */ + /* + * Allow delegation to the same record (representing a + * principal), even if by a different name. The easy and safe + * way to prove this is by SID comparison + */ if (!(orig_sid && target_sid && dom_sid_equal(orig_sid, target_sid))) { - talloc_free(mem_ctx); + talloc_free(frame); return KRB5KDC_ERR_BADOPTION; } - talloc_free(mem_ctx); - return ret; + talloc_free(frame); + return 0; } /* Certificates printed by a the Certificate Authority might have a diff --git a/source4/kdc/db-glue.h b/source4/kdc/db-glue.h index aa630f5d349..cadfac1deb8 100644 --- a/source4/kdc/db-glue.h +++ b/source4/kdc/db-glue.h @@ -40,9 +40,8 @@ krb5_error_code samba_kdc_nextkey(krb5_context context, krb5_error_code samba_kdc_check_s4u2self(krb5_context context, - struct samba_kdc_db_context *kdc_db_ctx, - struct samba_kdc_entry *skdc_entry, - krb5_const_principal target_principal); + struct samba_kdc_entry *skdc_entry_client, + struct samba_kdc_entry *skdc_entry_server_target); krb5_error_code samba_kdc_check_pkinit_ms_upn_match(krb5_context context, diff --git a/source4/kdc/hdb-samba4.c b/source4/kdc/hdb-samba4.c index 38ce9807c02..f0939193ad7 100644 --- a/source4/kdc/hdb-samba4.c +++ b/source4/kdc/hdb-samba4.c @@ -274,38 +274,19 @@ hdb_samba4_check_pkinit_ms_upn_match(krb5_context context, HDB *db, static krb5_error_code hdb_samba4_check_s4u2self(krb5_context context, HDB *db, - hdb_entry_ex *entry, - krb5_const_principal target_principal) + hdb_entry_ex *client_entry, + hdb_entry_ex *server_target_entry) { - struct samba_kdc_db_context *kdc_db_ctx; - struct samba_kdc_entry *skdc_entry; - krb5_error_code ret; - - kdc_db_ctx = talloc_get_type_abort(db->hdb_db, - struct samba_kdc_db_context); - skdc_entry = talloc_get_type_abort(entry->ctx, - struct samba_kdc_entry); - - ret = samba_kdc_check_s4u2self(context, kdc_db_ctx, - skdc_entry, - target_principal); - switch (ret) { - case 0: - break; - case SDB_ERR_WRONG_REALM: - ret = HDB_ERR_WRONG_REALM; - break; - case SDB_ERR_NOENTRY: - ret = HDB_ERR_NOENTRY; - break; - case SDB_ERR_NOT_FOUND_HERE: - ret = HDB_ERR_NOT_FOUND_HERE; - break; - default: - break; - } - - return ret; + struct samba_kdc_entry *skdc_client_entry + = talloc_get_type_abort(client_entry->ctx, + struct samba_kdc_entry); + struct samba_kdc_entry *skdc_server_target_entry + = talloc_get_type_abort(server_target_entry->ctx, + struct samba_kdc_entry); + + return samba_kdc_check_s4u2self(context, + skdc_client_entry, + skdc_server_target_entry); } static void reset_bad_password_netlogon(TALLOC_CTX *mem_ctx, -- 2.34.1