krb5 commit [krb5-1.16]: Fix PKINIT rule matching against UPN SANs
Greg Hudson <[email protected]>
| Newsgroups | gmane.comp.encryption.kerberos.cvs |
|---|---|
| Message-ID | <[email protected]> |
https://github.com/krb5/krb5/commit/67632329dbacf7b1964df01a88f061d2f16063ef commit 67632329dbacf7b1964df01a88f061d2f16063ef Author: Greg Hudson <[email protected]> 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>^[email protected]$' +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])