From: Greg Hudson Date: Thu, 22 Mar 2018 23:46:22 +0000 (-0400) Subject: Fix PKINIT rule matching against UPN SANs X-Git-Tag: krb5-1.17-beta1~133 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0f26c1c7504777d6e7bfa1d3dee575c504ab6c05;p=thirdparty%2Fkrb5.git Fix PKINIT rule matching against UPN SANs Commit 46ff765e1fb8cbec2bb602b43311269e695dbedc (for ticket 8528) broke rule-based matching of UPN SANs using the 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 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 1110545777..0acb731cd6 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 b4bfd635bd..5ff81d8cf4 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 "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 c1ce84b826..c2a4c084d6 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 bbfde34b2c..76ad5bf194 100644 --- 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 67e0caeb41..7f95206c0b 100644 --- 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 769ad6a981..b97ff83633 100755 --- 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 = '^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 = 'CN=user$digitalSignature,keyEncipherment' realm.run([kadminl, 'setstr', realm.user_princ, 'pkinit_cert_match', rule])