sin/samba-mirror
1
0
Fork 0
mirror of https://github.com/samba-team/samba.git synced 2024-12-22 13:34:15 +03:00
Code

CVE-2020-25719 kdc: Avoid races and multiple DB lookups in s4u2self check

Browse Source 
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 <abartlet@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>
This commit is contained in:
Andrew Bartlett 2021-10-08 08:29:51 +13:00 committed by Jule Anger
parent 80257fa37c
commit 05898cfb13
5 changed files with 51 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
source4/heimdal/kdc/krb5tgs.c
View File

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

2
source4/heimdal/lib/hdb/hdb.h
View File

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

54
source4/kdc/db-glue.c
View File

@ -2510,53 +2510,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 *frame = talloc_stackframe();
TALLOC_CTX *mem_ctx = talloc_named(kdc_db_ctx, 0, "samba_kdc_check_s4u2self");
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");
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;
}
orig_sid = samdb_result_dom_sid(mem_ctx, skdc_entry->msg, "objectSid");
target_sid = samdb_result_dom_sid(mem_ctx, 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

5
source4/kdc/db-glue.h
View File

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

41
source4/kdc/hdb-samba4.c
View File

@ -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;
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);
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;
return samba_kdc_check_s4u2self(context,
skdc_client_entry,
skdc_server_target_entry);
}
static void reset_bad_password_netlogon(TALLOC_CTX *mem_ctx,