From ea8e8a4a8a8bca03a6ec9756a4422b47c0082454 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Nov 2021 12:51:28 +1300 Subject: [PATCH] kdc: Avoid races and multiple DB lookups in s4u2self check Assists Samba to address CVE-2020-25719 Passing in target_server as a string principal means that for an alias we must looking up the DB twice. This 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. We also avoid doing a name comparison if the HDB plugin provides a validation hook, this allows the HDB layer more freedom to choose how to handle things. In Samba AD the client record has already been bound to the the original client by the SID check in the PAC, so the record is known to match the ticket. Likewise by looking up server only once we ensure that the keys looked up originally (to decrypt) are in the record we confirm the SID for here. Samba BUG: https://bugzilla.samba.org/show_bug.cgi?id=14686 Signed-off-by: Andrew Bartlett (Based on Samba commit 05898cfb139ae0674c8251acc9d64c4c3d4c8376) --- kdc/krb5tgs.c | 26 ++++++++++++++++++-------- lib/hdb/hdb.h | 2 +- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/kdc/krb5tgs.c b/kdc/krb5tgs.c index 74d3ae9e7..16dae8f39 100644 --- a/kdc/krb5tgs.c +++ b/kdc/krb5tgs.c @@ -350,7 +350,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 @@ -358,18 +358,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; } @@ -1993,7 +2003,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, 4, "S4U2Self: %s is not allowed " "to impersonate to service " diff --git a/lib/hdb/hdb.h b/lib/hdb/hdb.h index 539312b01..a50c6d42c 100644 --- a/lib/hdb/hdb.h +++ b/lib/hdb/hdb.h @@ -294,7 +294,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 *); /** * Enable/disable synchronous updates