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 <abartlet@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>

diff --git a/source4/heimdal/kdc/krb5tgs.c b/source4/heimdal/kdc/krb5tgs.c
index 301ca92091a97209b27255b299a148d4ae16105e..d4a1c78e153c1e8ff6ad3da870363d74e04bf57c 100644 (file)
--- 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 6a09ecb6fe180ffd27f180cf2bf650be008e21ce..5ef9d9565f3fe8fe33c9c4c3b0c38b209db23608 100644 (file)
--- 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 5fd0f431cdfefb4b06bf2e7d2f6109076830722f..d55bf1663d467d02c190d12533b930839d81028a 100644 (file)
--- 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 aa630f5d349110b0011a2125a3c73104f1a762b7..cadfac1deb86bb29f8fe96d12d2a72ff9035fb98 100644 (file)
--- 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 38ce9807c02945552babedaedc676ed13ee3106e..f0939193ad78227c93d75792f9cab816c4c437b2 100644 (file)
--- 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,