krb5 commit [krb5-1.16]: Fix PKINIT rule matching against UPN SANs

Greg Hudson ghudson at mit.edu
Wed May 2 01:25:44 EDT 2018


https://github.com/krb5/krb5/commit/67632329dbacf7b1964df01a88f061d2f16063ef
commit 67632329dbacf7b1964df01a88f061d2f16063ef
Author: Greg Hudson <ghudson at mit.edu>
Date:   Thu Mar 22 19:46:22 2018 -0400

    Fix PKINIT rule matching against UPN SANs
    
    Commit 46ff765e1fb8cbec2bb602b43311269e695dbedc (for ticket 8528)
    broke rule-based matching of UPN SANs using the <SAN> rule type.  To
    fix this regression, make crypto_retrieve_cert_sans() return UPN SANs
    in their original string form, and only parse them into principal
    names in pkinit_srv.c:verify_client_san().  In
    pkinit_cert_matching_data, store UPN SANs as strings separately from
    PKINIT SANs instead of concatenating them together, and match original
    UPN strings against <SAN> rule regexps.  Add a test case.
    
    (cherry picked from commit 0f26c1c7504777d6e7bfa1d3dee575c504ab6c05)
    
    ticket: 8670
    version_fixed: 1.16.1

 src/plugins/preauth/pkinit/pkinit_crypto.h         |    6 +-
 src/plugins/preauth/pkinit/pkinit_crypto_openssl.c |   63 ++++----------------
 src/plugins/preauth/pkinit/pkinit_matching.c       |   20 ++++---
 src/plugins/preauth/pkinit/pkinit_srv.c            |   21 ++++--
 src/plugins/preauth/pkinit/pkinit_trace.h          |    3 +
 src/tests/t_pkinit.py                              |    7 ++
 6 files changed, 52 insertions(+), 68 deletions(-)

diff --git a/src/plugins/preauth/pkinit/pkinit_crypto.h b/src/plugins/preauth/pkinit/pkinit_crypto.h
index 2d3733b..4e4752f 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto.h
+++ b/src/plugins/preauth/pkinit/pkinit_crypto.h
@@ -97,8 +97,8 @@ typedef struct _pkinit_cert_matching_data {
     char *issuer_dn;	    /* rfc2253-style issuer name string */
     unsigned int ku_bits;   /* key usage information */
     unsigned int eku_bits;  /* extended key usage information */
-    krb5_principal *sans;   /* Null-terminated array of subject alternative
-			       name info (pkinit and ms-upn) */
+    krb5_principal *sans;   /* Null-terminated array of PKINIT SANs */
+    char **upns;	    /* Null-terimnated array of UPN SANs */
 } pkinit_cert_matching_data;
 
 /*
@@ -250,7 +250,7 @@ krb5_error_code crypto_retrieve_cert_sans
 		    if non-NULL, a null-terminated array of
 		    id-pkinit-san values found in the certificate
 		    are returned */
-	krb5_principal **upn_sans,			/* OUT
+	char ***upn_sans,				/* OUT
 		    if non-NULL, a null-terminated array of
 		    id-ms-upn-san values found in the certificate
 		    are returned */
diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
index 0c8dd7e..2064eb7 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
+++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
@@ -29,6 +29,7 @@
  * SUCH DAMAGES.
  */
 
+#include "k5-int.h"
 #include "pkinit_crypto_openssl.h"
 #include "k5-buf.h"
 #include <dlfcn.h>
@@ -2083,15 +2084,14 @@ crypto_retrieve_X509_sans(krb5_context context,
                           pkinit_plg_crypto_context plgctx,
                           pkinit_req_crypto_context reqctx,
                           X509 *cert,
-                          krb5_principal **princs_ret,
-                          krb5_principal **upn_ret,
+                          krb5_principal **princs_ret, char ***upn_ret,
                           unsigned char ***dns_ret)
 {
     krb5_error_code retval = EINVAL;
     char buf[DN_BUF_LEN];
     int p = 0, u = 0, d = 0, ret = 0, l;
     krb5_principal *princs = NULL;
-    krb5_principal *upns = NULL;
+    char **upns = NULL;
     unsigned char **dnss = NULL;
     unsigned int i, num_found = 0, num_sans = 0;
     X509_EXTENSION *ext = NULL;
@@ -2141,7 +2141,7 @@ crypto_retrieve_X509_sans(krb5_context context,
         }
     }
     if (upn_ret != NULL) {
-        upns = calloc(num_sans + 1, sizeof(krb5_principal));
+        upns = calloc(num_sans + 1, sizeof(*upns));
         if (upns == NULL) {
             retval = ENOMEM;
             goto cleanup;
@@ -2184,16 +2184,9 @@ crypto_retrieve_X509_sans(krb5_context context,
                 /* Prevent abuse of embedded null characters. */
                 if (memchr(name.data, '\0', name.length))
                     break;
-                ret = krb5_parse_name_flags(context, name.data,
-                                            KRB5_PRINCIPAL_PARSE_ENTERPRISE,
-                                            &upns[u]);
-                if (ret) {
-                    pkiDebug("%s: failed parsing ms-upn san value\n",
-                             __FUNCTION__);
-                } else {
-                    u++;
-                    num_found++;
-                }
+                upns[u] = k5memdup0(name.data, name.length, &ret);
+                if (upns[u] == NULL)
+                    goto cleanup;
             } else {
                 pkiDebug("%s: unrecognized othername oid in SAN\n",
                          __FUNCTION__);
@@ -2245,7 +2238,7 @@ cleanup:
         krb5_free_principal(context, princs[i]);
     free(princs);
     for (i = 0; upns != NULL && upns[i] != NULL; i++)
-        krb5_free_principal(context, upns[i]);
+        free(upns[i]);
     free(upns);
     for (i = 0; dnss != NULL && dnss[i] != NULL; i++)
         free(dnss[i]);
@@ -2269,8 +2262,7 @@ crypto_retrieve_cert_sans(krb5_context context,
                           pkinit_plg_crypto_context plgctx,
                           pkinit_req_crypto_context reqctx,
                           pkinit_identity_crypto_context idctx,
-                          krb5_principal **princs_ret,
-                          krb5_principal **upn_ret,
+                          krb5_principal **princs_ret, char ***upn_ret,
                           unsigned char ***dns_ret)
 {
     krb5_error_code retval = EINVAL;
@@ -5061,6 +5053,9 @@ crypto_cert_free_matching_data(krb5_context context,
     for (i = 0; md->sans != NULL && md->sans[i] != NULL; i++)
         krb5_free_principal(context, md->sans[i]);
     free(md->sans);
+    for (i = 0; md->upns != NULL && md->upns[i] != NULL; i++)
+        free(md->upns[i]);
+    free(md->upns);
     free(md);
 }
 
@@ -5089,8 +5084,6 @@ get_matching_data(krb5_context context,
 {
     krb5_error_code ret = ENOMEM;
     pkinit_cert_matching_data *md = NULL;
-    krb5_principal *pkinit_sans = NULL, *upn_sans = NULL;
-    size_t i, j;
 
     *md_out = NULL;
 
@@ -5107,40 +5100,10 @@ get_matching_data(krb5_context context,
 
     /* Get the SAN data. */
     ret = crypto_retrieve_X509_sans(context, plg_cryptoctx, req_cryptoctx,
-                                    cert, &pkinit_sans, &upn_sans, NULL);
+                                    cert, &md->sans, &md->upns, NULL);
     if (ret)
         goto cleanup;
 
-    j = 0;
-    if (pkinit_sans != NULL) {
-        for (i = 0; pkinit_sans[i] != NULL; i++)
-            j++;
-    }
-    if (upn_sans != NULL) {
-        for (i = 0; upn_sans[i] != NULL; i++)
-            j++;
-    }
-    if (j != 0) {
-        md->sans = calloc((size_t)j+1, sizeof(*md->sans));
-        if (md->sans == NULL) {
-            ret = ENOMEM;
-            goto cleanup;
-        }
-        j = 0;
-        if (pkinit_sans != NULL) {
-            for (i = 0; pkinit_sans[i] != NULL; i++)
-                md->sans[j++] = pkinit_sans[i];
-            free(pkinit_sans);
-        }
-        if (upn_sans != NULL) {
-            for (i = 0; upn_sans[i] != NULL; i++)
-                md->sans[j++] = upn_sans[i];
-            free(upn_sans);
-        }
-        md->sans[j] = NULL;
-    } else
-        md->sans = NULL;
-
     /* Get the KU and EKU data. */
     ret = crypto_retrieve_X509_key_usage(context, plg_cryptoctx,
                                          req_cryptoctx, cert, &md->ku_bits,
diff --git a/src/plugins/preauth/pkinit/pkinit_matching.c b/src/plugins/preauth/pkinit/pkinit_matching.c
index c1ce84b..c2a4c08 100644
--- a/src/plugins/preauth/pkinit/pkinit_matching.c
+++ b/src/plugins/preauth/pkinit/pkinit_matching.c
@@ -470,7 +470,6 @@ component_match(krb5_context context,
 {
     int match = 0;
     int i;
-    krb5_principal p;
     char *princ_string;
 
     switch (rc->kwval_type) {
@@ -483,15 +482,18 @@ component_match(krb5_context context,
             match = regexp_match(context, rc, md->issuer_dn);
             break;
         case kw_san:
-            if (md->sans == NULL)
-                break;
-            for (i = 0, p = md->sans[i]; p != NULL; p = md->sans[++i]) {
-                krb5_unparse_name(context, p, &princ_string);
+            for (i = 0; md->sans != NULL && md->sans[i] != NULL; i++) {
+                krb5_unparse_name(context, md->sans[i], &princ_string);
                 match = regexp_match(context, rc, princ_string);
                 krb5_free_unparsed_name(context, princ_string);
                 if (match)
                     break;
             }
+            for (i = 0; md->upns != NULL && md->upns[i] != NULL; i++) {
+                match = regexp_match(context, rc, md->upns[i]);
+                if (match)
+                    break;
+            }
             break;
         default:
             pkiDebug("%s: keyword %s, keyword value %s mismatch\n",
@@ -572,12 +574,14 @@ check_all_certs(krb5_context context,
         pkiDebug("%s: subject: '%s'\n", __FUNCTION__, md->subject_dn);
 #if 0
         pkiDebug("%s: issuer:  '%s'\n", __FUNCTION__, md->subject_dn);
-        for (j = 0, p = md->sans[j]; p != NULL; p = md->sans[++j]) {
+        for (j = 0; md->sans != NULL && md->sans[j] != NULL; j++) {
             char *san_string;
-            krb5_unparse_name(context, p, &san_string);
-            pkiDebug("%s: san: '%s'\n", __FUNCTION__, san_string);
+            krb5_unparse_name(context, md->sans[j], &san_string);
+            pkiDebug("%s: PKINIT san: '%s'\n", __FUNCTION__, san_string);
             krb5_free_unparsed_name(context, san_string);
         }
+        for (j = 0; md->upns != NULL && md->upns[j] != NULL; j++)
+            pkiDebug("%s: UPN san: '%s'\n", __FUNCTION__, md->upns[j]);
 #endif
         certs_checked++;
         for (rc = rs->crs; rc != NULL; rc = rc->next) {
diff --git a/src/plugins/preauth/pkinit/pkinit_srv.c b/src/plugins/preauth/pkinit/pkinit_srv.c
index 4e96858..8aa4d8b 100644
--- a/src/plugins/preauth/pkinit/pkinit_srv.c
+++ b/src/plugins/preauth/pkinit/pkinit_srv.c
@@ -174,8 +174,9 @@ verify_client_san(krb5_context context,
                   int *valid_san)
 {
     krb5_error_code retval;
-    krb5_principal *princs = NULL;
-    krb5_principal *upns = NULL;
+    krb5_principal *princs = NULL, upn;
+    krb5_boolean match;
+    char **upns = NULL;
     int i;
 #ifdef DEBUG_SAN_INFO
     char *client_string = NULL, *san_string;
@@ -251,12 +252,18 @@ verify_client_san(krb5_context context,
     pkiDebug("%s: Checking upn sans\n", __FUNCTION__);
     for (i = 0; upns[i] != NULL; i++) {
 #ifdef DEBUG_SAN_INFO
-        krb5_unparse_name(context, upns[i], &san_string);
         pkiDebug("%s: Comparing client '%s' to upn san value '%s'\n",
-                 __FUNCTION__, client_string, san_string);
-        krb5_free_unparsed_name(context, san_string);
+                 __FUNCTION__, client_string, upns[i]);
 #endif
-        if (cb->match_client(context, rock, upns[i])) {
+        retval = krb5_parse_name_flags(context, upns[i],
+                                       KRB5_PRINCIPAL_PARSE_ENTERPRISE, &upn);
+        if (retval) {
+            TRACE_PKINIT_SERVER_UPN_PARSE_FAIL(context, upns[i], retval);
+            continue;
+        }
+        match = cb->match_client(context, rock, upn);
+        krb5_free_principal(context, upn);
+        if (match) {
             TRACE_PKINIT_SERVER_MATCHING_UPN_FOUND(context);
             *valid_san = 1;
             retval = 0;
@@ -282,7 +289,7 @@ out:
     }
     if (upns != NULL) {
         for (i = 0; upns[i] != NULL; i++)
-            krb5_free_principal(context, upns[i]);
+            free(upns[i]);
         free(upns);
     }
 #ifdef DEBUG_SAN_INFO
diff --git a/src/plugins/preauth/pkinit/pkinit_trace.h b/src/plugins/preauth/pkinit/pkinit_trace.h
index d4eb39d..2d95da9 100644
--- a/src/plugins/preauth/pkinit/pkinit_trace.h
+++ b/src/plugins/preauth/pkinit/pkinit_trace.h
@@ -121,6 +121,9 @@
     TRACE(c, "PKINIT server returning PA data")
 #define TRACE_PKINIT_SERVER_SAN_REJECT(c)                               \
     TRACE(c, "PKINIT server found no acceptable SAN in client cert")
+#define TRACE_PKINIT_SERVER_UPN_PARSE_FAIL(c, upn, ret)                 \
+    TRACE(c, "PKINIT server could not parse UPN \"{str}\": {kerr}",     \
+          upn, ret)
 
 #define TRACE_PKINIT_EKU(c)                                             \
     TRACE(c, "PKINIT found acceptable EKU and digitalSignature KU")
diff --git a/src/tests/t_pkinit.py b/src/tests/t_pkinit.py
index b790a7c..86fe661 100755
--- a/src/tests/t_pkinit.py
+++ b/src/tests/t_pkinit.py
@@ -301,6 +301,13 @@ realm.kinit(realm.user_princ,
             flags=['-X', 'X509_user_identity=%s' % p12_identity])
 realm.klist(realm.user_princ)
 
+# Regression test for #8670: match a UPN SAN with a single rule.
+rule = '<SAN>^user at krbtest.com$'
+realm.run([kadminl, 'setstr', realm.user_princ, 'pkinit_cert_match', rule])
+realm.kinit(realm.user_princ,
+            flags=['-X', 'X509_user_identity=%s' % p12_upn_identity])
+realm.klist(realm.user_princ)
+
 # Match a combined rule (default prefix is &&).
 rule = '<SUBJECT>CN=user$<KU>digitalSignature,keyEncipherment'
 realm.run([kadminl, 'setstr', realm.user_princ, 'pkinit_cert_match', rule])


More information about the cvs-krb5 mailing list