oysteikt/heimdal
0
0
Fork 0
Code Issues 5 Pull Requests Releases Activity

kdc: Avoid races and multiple DB lookups in s4u2self check

Browse Source 
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 <abartlet@samba.org>

(Based on Samba commit 05898cfb139ae0674c8251acc9d64c4c3d4c8376)
This commit is contained in:
Joseph Sutton
2021-11-16 12:51:28 +13:00
committed by Luke Howard
parent 6b635f66de
commit ea8e8a4a8a
2 changed files with 19 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
kdc/krb5tgs.c
View File

@@ -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 "

2
lib/hdb/hdb.h
View File

@@ -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