krb5 commit: Fix PKINIT rule matching against UPN SANs
Greg Hudson
ghudson at mit.edu
Wed Apr 25 12:01:48 EDT 2018
https://github.com/krb5/krb5/commit/0f26c1c7504777d6e7bfa1d3dee575c504ab6c05
commit 0f26c1c7504777d6e7bfa1d3dee575c504ab6c05
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.
ticket: 8670
tags: pullup
target_version: 1.16-next
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 1110545..0acb731 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 b4bfd63..5ff81d8 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 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 bbfde34..76ad5bf 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 67e0cae..7f95206 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 769ad6a..b97ff83 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 = '<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