krb5 commit: Verify PAC client name independently of name-type

[Greg Hudson]

https://github.com/krb5/krb5/commit/e935913a4dc9461c129e373bfd752e8a6c795e28
commit e935913a4dc9461c129e373bfd752e8a6c795e28
Author: Isaac Boukris <iboukris at gmail.com>
Date:   Mon Jun 10 15:33:06 2019 +0300

    Verify PAC client name independently of name-type
    
    In krb5_pac_verify(), unparse the provided principal name and compare
    using strcmp(), instead of parsing pac principal, in order to avoid
    relying on the provided name type.
    
    This change is needed for tickets issued with cross-realm S4U2Proxy
    (with resource-based constrained delegation), because the final
    request uses a cross-TGT as the evidence ticket, so the ticket client
    name is taken from the PAC and does not preserve the name type.
    Microsoft KDCs use NT-MS-PRINCIPAL as the ticket client name type in
    this case, regardless of the original name type.
    
    [ghudson at mit.edu: rewrote commit message; made minor style edits]
    
    ticket: 8815 (new)

 src/lib/krb5/krb/pac.c   |   29 +++++++-------------------
 src/lib/krb5/krb/t_pac.c |   49 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/lib/krb5/krb/pac.c b/src/lib/krb5/krb/pac.c
index cc74f37..5efc91e 100644
--- a/src/lib/krb5/krb/pac.c
+++ b/src/lib/krb5/krb/pac.c
@@ -408,12 +408,11 @@ k5_pac_validate_client(krb5_context context,
 {
     krb5_error_code ret;
     krb5_data client_info;
-    char *pac_princname;
+    char *pac_princname, *princname;
     unsigned char *p;
     krb5_timestamp pac_authtime;
     krb5_ui_2 pac_princname_length;
     int64_t pac_nt_authtime;
-    krb5_principal pac_principal;
     int flags = 0;
 
     ret = k5_pac_locate_buffer(context, pac, KRB5_PAC_CLIENT_INFO,
@@ -442,33 +441,21 @@ k5_pac_validate_client(krb5_context context,
     if (ret != 0)
         return ret;
 
-    /* Parse the UTF-8 name as an enterprise principal if we are matching
-     * against one; otherwise parse it as a regular principal. */
-    if (principal->type == KRB5_NT_ENTERPRISE_PRINCIPAL)
-        flags |= KRB5_PRINCIPAL_PARSE_ENTERPRISE;
+    flags = KRB5_PRINCIPAL_UNPARSE_DISPLAY;
+    if (!with_realm)
+        flags |= KRB5_PRINCIPAL_UNPARSE_NO_REALM;
 
-    if (with_realm)
-        flags |= KRB5_PRINCIPAL_PARSE_REQUIRE_REALM;
-    else
-        flags |= KRB5_PRINCIPAL_PARSE_NO_REALM;
-
-    ret = krb5_parse_name_flags(context, pac_princname, flags, &pac_principal);
+    ret = krb5_unparse_name_flags(context, principal, flags, &princname);
     if (ret != 0) {
         free(pac_princname);
         return ret;
     }
 
-    free(pac_princname);
-
-    if (pac_authtime != authtime ||
-        !krb5_principal_compare_flags(context,
-                                      pac_principal,
-                                      principal,
-                                      with_realm ? 0 :
-                                      KRB5_PRINCIPAL_COMPARE_IGNORE_REALM))
+    if (pac_authtime != authtime || strcmp(pac_princname, princname) != 0)
         ret = KRB5KRB_AP_WRONG_PRINC;
 
-    krb5_free_principal(context, pac_principal);
+    free(pac_princname);
+    krb5_free_unparsed_name(context, princname);
 
     return ret;
 }
diff --git a/src/lib/krb5/krb/t_pac.c b/src/lib/krb5/krb/t_pac.c
index 7b756a2..ee47152 100644
--- a/src/lib/krb5/krb/t_pac.c
+++ b/src/lib/krb5/krb/t_pac.c
@@ -733,13 +733,18 @@ main(int argc, char **argv)
     }
 
     {
-        krb5_principal ep;
+        krb5_principal ep, np;
 
         ret = krb5_parse_name_flags(context, user,
                                     KRB5_PRINCIPAL_PARSE_ENTERPRISE, &ep);
         if (ret)
             err(context, ret, "krb5_parse_name_flags");
 
+        ret = krb5_copy_principal(context, ep, &np);
+        if (ret)
+            err(context, ret, "krb5_copy_principal");
+        np->type = KRB5_NT_MS_PRINCIPAL;
+
         /* Try to verify as enterprise. */
         ret = krb5_pac_verify(context, pac, authtime, ep, &member_keyblock,
                               &kdc_keyblock);
@@ -788,6 +793,47 @@ main(int argc, char **argv)
         if (ret)
             err(context, ret, "krb5_pac_verify enterprise failed");
 
+        /* Also verify enterprise as KRB5_NT_MS_PRINCIPAL. */
+        ret = krb5_pac_verify(context, pac, authtime, np, &member_keyblock,
+                              &kdc_keyblock);
+        if (ret)
+            err(context, ret, "krb5_pac_verify enterprise as nt-ms failed");
+
+        ret = krb5_pac_verify(context, pac, authtime, p, &member_keyblock,
+                              &kdc_keyblock);
+        if (!ret)
+            err(context, ret, "krb5_pac_verify should have failed");
+
+        krb5_pac_free(context, pac);
+
+        /* Test nt-ms-principal. */
+        ret = krb5_pac_init(context, &pac);
+        if (ret)
+            err(context, ret, "krb5_pac_init");
+
+        ret = krb5_pac_sign(context, pac, authtime, np, &member_keyblock,
+                            &kdc_keyblock, &data);
+        if (ret)
+            err(context, ret, "krb5_pac_sign enterprise failed");
+
+        krb5_pac_free(context, pac);
+
+        ret = krb5_pac_parse(context, data.data, data.length, &pac);
+        krb5_free_data_contents(context, &data);
+        if (ret)
+            err(context, ret, "krb5_pac_parse failed");
+
+        ret = krb5_pac_verify(context, pac, authtime, np, &member_keyblock,
+                              &kdc_keyblock);
+        if (ret)
+            err(context, ret, "krb5_pac_verify enterprise failed");
+
+        /* Also verify as enterprise principal. */
+        ret = krb5_pac_verify(context, pac, authtime, ep, &member_keyblock,
+                              &kdc_keyblock);
+        if (ret)
+            err(context, ret, "krb5_pac_verify nt-ms as enterprise failed");
+
         ret = krb5_pac_verify(context, pac, authtime, p, &member_keyblock,
                               &kdc_keyblock);
         if (!ret)
@@ -862,6 +908,7 @@ main(int argc, char **argv)
             err(context, ret, "krb5_pac_verify_ext should have failed");
 
         krb5_free_principal(context, ep);
+        krb5_free_principal(context, np);
     }
 
     krb5_pac_free(context, pac);