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.

ticket: 8670
tags: pullup
target_version: 1.16-next

diff --git a/src/plugins/preauth/pkinit/pkinit_crypto.h b/src/plugins/preauth/pkinit/pkinit_crypto.h
index 11105457772221f4e430eec72432401cb87e3840..0acb731cd6adde6b8c445ce274ae8721f269098a 100644 (file)
--- 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 b4bfd635bd77b3e91fbe1349c14ab51577270ce4..5ff81d8cf4bef51c804c9876ecdfc2f34fc93090 100644 (file)
--- 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 "k5-hex.h"
@@ -2084,15 +2085,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;
@@ -2142,7 +2142,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;
@@ -2185,16 +2185,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__);
@@ -2246,7 +2239,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]);
@@ -2270,8 +2263,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;
@@ -5069,6 +5061,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);
 }
 
@@ -5097,8 +5092,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;
 
@@ -5115,40 +5108,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 c1ce84b8264151908d8cf695722285306f759b50..c2a4c084d68fc1eb0bbe885e276199c3217ae601 100644 (file)
--- 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 bbfde34b2cc30e1ec51d058bf9cf48bb5c829ece..76ad5bf194578042748bf815450397c41c2010ae 100644 (file)
--- a/src/plugins/preauth/pkinit/pkinit_srv.c
+++ b/src/plugins/preauth/pkinit/pkinit_srv.c
@@ -178,8 +178,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;
@@ -255,12 +256,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;
@@ -286,7 +293,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 67e0caeb41fc13e9368d52d767bf0fbe35fda6e9..7f95206c0b11eb5058cc7b7315cdbd5609b2b327 100644 (file)
--- a/src/plugins/preauth/pkinit/pkinit_trace.h
+++ b/src/plugins/preauth/pkinit/pkinit_trace.h
@@ -123,6 +123,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 769ad6a981e6c169c72c83796890249605813cc5..b97ff83633f092e45c900b421f4f9b022faf6b7e 100755 (executable)
--- a/src/tests/t_pkinit.py
+++ b/src/tests/t_pkinit.py
@@ -349,6 +349,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@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])