| Message ID | 20210604143125.4946-1-heiko.wundram@gehrkens.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Implement Windows CA template match for Crypto-API selector | expand |
Hi, On Fri, Jun 04, 2021 at 04:31:25PM +0200, Heiko Wundram wrote: > The certificate selection process for the Crypto API certificates > is currently fixed to match on subject or identifier. Especially > if certificates that are used for OpenVPN are managed by a Windows CA, [..] Just for completeness - I assume that this is a v2 of the patch, and "something was changed". Since we're all very lazy^Wbusy people, it would be good to include a list of v2 changes in the commit message, like this: v2: - included suggestions about windows dunno things handling - code restructuring in some_function() and please use "git send-email -v2 ..." when sending it, so git can put a "v2" in the subject line, and patchwork can tell me "ah, this is the new version". Re-sending the patch is not needed in this case, but it would be nice if you could send a quick list of changes in v2. thanks, gert
Hi, Oh, it seems I replied to the stale thread (version 1). As Gert mentioned, please include a version tag and use --in-reply-to <msg-id> to keep it threaded in the next iteration. Some additional comments below. On Fri, Jun 4, 2021 at 10:41 AM Heiko Wundram <heiko.wundram@gehrkens.it> wrote: > > The certificate selection process for the Crypto API certificates > is currently fixed to match on subject or identifier. Especially > if certificates that are used for OpenVPN are managed by a Windows CA, > it is appropriate to select the certificate to use by the template > that it is generated from, especially on domain-joined clients which > automatically acquire/renew the corresponding certificate. > > The attached match implements the match on TMPL: with either a template > name (which is looked up through CryptFindOIDInfo) or by specifying the > OID of the template directly, which then is matched against the > corresponding X509 extensions specifying the template that the certificate > was generated from. > > The logic requires to walk all certificates in the underlying store and > to match the certificate extensions directly. The hook which is > implemented in the certificate selection logic is generic to allow > other Crypto-API certificate matches to also be implemented at some > point in the future. > > The logic to match the certificate template is taken from the > implementation in the .NET core runtime, see Pal.Windows/FindPal.cs in > in the implementation of System.Security.Cryptography.X509Certificates. > > Signed-off-by: Heiko Wundram <heiko.wundram@gehrkens.it> > --- > src/openvpn/cryptoapi.c | 142 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 135 insertions(+), 7 deletions(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index ded8c914..3532a9a0 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -732,6 +732,115 @@ err: > > #endif /* OPENSSL_VERSION_NUMBER >= 1.1.0 */ > > +static void * > +decode_object(struct gc_arena *gc, LPCSTR struct_type, const CRYPT_OBJID_BLOB *val, DWORD flags, DWORD *cb) > +{ > + /* get byte count for decoding */ > + BYTE *buf; > + if (!CryptDecodeObject(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, struct_type, val->pbData, val->cbData, flags, NULL, cb)) > + { > + return NULL; > + } > + > + /* do the actual decode */ > + buf = gc_malloc(*cb, false, gc); > + if (!CryptDecodeObject(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, struct_type, val->pbData, val->cbData, flags, buf, cb)) > + { > + return NULL; > + } > + > + return buf; > +} > + > +static const CRYPT_OID_INFO * > +find_oid(DWORD keytype, const void *key, DWORD groupid, bool fallback) > +{ > + const CRYPT_OID_INFO *info = NULL; > + > + /* force resolve from local as first step */ > + if (groupid != CRYPT_HASH_ALG_OID_GROUP_ID > + && groupid != CRYPT_ENCRYPT_ALG_OID_GROUP_ID > + && groupid != CRYPT_PUBKEY_ALG_OID_GROUP_ID > + && groupid != CRYPT_SIGN_ALG_OID_GROUP_ID > + && groupid != CRYPT_RDN_ATTR_OID_GROUP_ID > + && groupid != CRYPT_EXT_OR_ATTR_OID_GROUP_ID > + && groupid != CRYPT_KDF_OID_GROUP_ID) > + { > + info = CryptFindOIDInfo(keytype, (void*)key, groupid | CRYPT_OID_DISABLE_SEARCH_DS_FLAG); > + } See comment in the previous email. Essentially, the if clause is not needed. > + > + /* try proper resolve if not found yet, also including AD */ > + if (!info) > + { > + info = CryptFindOIDInfo(keytype, (void*)key, groupid); > + } > + > + /* fall back to all groups if not found yet and fallback requested */ > + if (!info && fallback && groupid) > + { > + info = CryptFindOIDInfo(keytype, (void*)key, 0); > + } > + > + return info; > +} > + > +static bool > +test_certificate_template(const char *cert_prop, const CERT_CONTEXT *cert_ctx) > +{ > + const CERT_INFO *info = cert_ctx->pCertInfo; > + const CERT_EXTENSION *ext; > + DWORD cbext; > + void *pvext; > + struct gc_arena gc = gc_new(); > + const WCHAR *tmpl_name = wide_string(cert_prop, &gc); > + > + /* check for V1 extension (Windows 2K and below) */ > + ext = CertFindExtension(szOID_ENROLL_CERTTYPE_EXTENSION, info->cExtension, info->rgExtension); > + if (ext) > + { > + pvext = decode_object(&gc, X509_UNICODE_ANY_STRING, &ext->Value, 0, &cbext); > + if (pvext && cbext >= sizeof(CERT_NAME_VALUE)) > + { > + const CERT_NAME_VALUE *nv = (const CERT_NAME_VALUE*)pvext; > + if (nv->Value.cbData >= sizeof(WCHAR) * (wcslen(tmpl_name) + 1) > + && !_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name)) > + { > + /* data content matches extension name */ > + gc_free(&gc); > + return true; > + } > + } > + } > + > + /* check for V2 extension (Windows 2003+) */ > + ext = CertFindExtension(szOID_CERTIFICATE_TEMPLATE, info->cExtension, info->rgExtension); > + if (ext) > + { > + pvext = decode_object(&gc, X509_CERTIFICATE_TEMPLATE, &ext->Value, 0, &cbext); > + if (pvext && cbext >= sizeof(CERT_TEMPLATE_EXT)) > + { > + const CERT_TEMPLATE_EXT *cte = (const CERT_TEMPLATE_EXT*)pvext; > + const CRYPT_OID_INFO *tmpl_oid = find_oid(CRYPT_OID_INFO_NAME_KEY, tmpl_name, CRYPT_TEMPLATE_OID_GROUP_ID, true); > + if (tmpl_oid && !stricmp(tmpl_oid->pszOID, cte->pszObjId)) > + { > + /* found OID match in extension against resolved key */ > + gc_free(&gc); > + return true; > + } > + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId)) > + { > + /* found direct OID match with certificate property specified */ > + gc_free(&gc); > + return true; > + } > + } > + } > + > + /* no extension found, exit */ > + gc_free(&gc); > + return false; > +} > + > static const CERT_CONTEXT * > find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > { > @@ -743,17 +852,25 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > * The first matching certificate that has not expired is returned. > */ > const CERT_CONTEXT *rv = NULL; > - DWORD find_type; > - const void *find_param; > + DWORD find_type = CERT_FIND_ANY; > + const void *find_param = NULL; > + bool (*find_test)(const char*, const CERT_CONTEXT*) = NULL; > unsigned char hash[255]; > CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash}; > struct gc_arena gc = gc_new(); > > - if (!strncmp(cert_prop, "SUBJ:", 5)) > + if (!strncmp(cert_prop, "TMPL:", 5)) > + { > + /* skip the tag */ > + cert_prop += 5; > + find_test = &test_certificate_template; > + } > + else if (!strncmp(cert_prop, "SUBJ:", 5)) > { > /* skip the tag */ > - find_param = wide_string(cert_prop + 5, &gc); > + cert_prop += 5; > find_type = CERT_FIND_SUBJECT_STR_W; > + find_param = wide_string(cert_prop, &gc); > } > else if (!strncmp(cert_prop, "THUMB:", 6)) > { > @@ -819,12 +936,23 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > { > validity = CertVerifyTimeValidity(NULL, rv->pCertInfo); > } > - if (!rv || validity == 0) > + else > { > break; > } > - msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store %s.", > - validity < 0 ? "not yet valid" : "that has expired"); > + > + if (validity == 0) > + { > + if (!find_test || find_test(cert_prop, rv)) > + { > + break; > + } > + } > + else > + { > + msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store %s.", > + validity < 0 ? "not yet valid" : "that has expired"); This logic is still not correct. This would warn on all expired certs when TMPL: match is in use. One way to fix this is to change the "else" to "else if (!find_test)". Any expired certs that could match the template will not be warned about. I think that's okay. It would be nice to amend the comment at the top of this function to include TMPL: And man page update. Looks good otherwise. I don't have a setup to test this, so only checking that this doesn't break anything else.I assume it works for you. Selva Selva
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index ded8c914..3532a9a0 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -732,6 +732,115 @@ err: #endif /* OPENSSL_VERSION_NUMBER >= 1.1.0 */ +static void * +decode_object(struct gc_arena *gc, LPCSTR struct_type, const CRYPT_OBJID_BLOB *val, DWORD flags, DWORD *cb) +{ + /* get byte count for decoding */ + BYTE *buf; + if (!CryptDecodeObject(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, struct_type, val->pbData, val->cbData, flags, NULL, cb)) + { + return NULL; + } + + /* do the actual decode */ + buf = gc_malloc(*cb, false, gc); + if (!CryptDecodeObject(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, struct_type, val->pbData, val->cbData, flags, buf, cb)) + { + return NULL; + } + + return buf; +} + +static const CRYPT_OID_INFO * +find_oid(DWORD keytype, const void *key, DWORD groupid, bool fallback) +{ + const CRYPT_OID_INFO *info = NULL; + + /* force resolve from local as first step */ + if (groupid != CRYPT_HASH_ALG_OID_GROUP_ID + && groupid != CRYPT_ENCRYPT_ALG_OID_GROUP_ID + && groupid != CRYPT_PUBKEY_ALG_OID_GROUP_ID + && groupid != CRYPT_SIGN_ALG_OID_GROUP_ID + && groupid != CRYPT_RDN_ATTR_OID_GROUP_ID + && groupid != CRYPT_EXT_OR_ATTR_OID_GROUP_ID + && groupid != CRYPT_KDF_OID_GROUP_ID) + { + info = CryptFindOIDInfo(keytype, (void*)key, groupid | CRYPT_OID_DISABLE_SEARCH_DS_FLAG); + } + + /* try proper resolve if not found yet, also including AD */ + if (!info) + { + info = CryptFindOIDInfo(keytype, (void*)key, groupid); + } + + /* fall back to all groups if not found yet and fallback requested */ + if (!info && fallback && groupid) + { + info = CryptFindOIDInfo(keytype, (void*)key, 0); + } + + return info; +} + +static bool +test_certificate_template(const char *cert_prop, const CERT_CONTEXT *cert_ctx) +{ + const CERT_INFO *info = cert_ctx->pCertInfo; + const CERT_EXTENSION *ext; + DWORD cbext; + void *pvext; + struct gc_arena gc = gc_new(); + const WCHAR *tmpl_name = wide_string(cert_prop, &gc); + + /* check for V1 extension (Windows 2K and below) */ + ext = CertFindExtension(szOID_ENROLL_CERTTYPE_EXTENSION, info->cExtension, info->rgExtension); + if (ext) + { + pvext = decode_object(&gc, X509_UNICODE_ANY_STRING, &ext->Value, 0, &cbext); + if (pvext && cbext >= sizeof(CERT_NAME_VALUE)) + { + const CERT_NAME_VALUE *nv = (const CERT_NAME_VALUE*)pvext; + if (nv->Value.cbData >= sizeof(WCHAR) * (wcslen(tmpl_name) + 1) + && !_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name)) + { + /* data content matches extension name */ + gc_free(&gc); + return true; + } + } + } + + /* check for V2 extension (Windows 2003+) */ + ext = CertFindExtension(szOID_CERTIFICATE_TEMPLATE, info->cExtension, info->rgExtension); + if (ext) + { + pvext = decode_object(&gc, X509_CERTIFICATE_TEMPLATE, &ext->Value, 0, &cbext); + if (pvext && cbext >= sizeof(CERT_TEMPLATE_EXT)) + { + const CERT_TEMPLATE_EXT *cte = (const CERT_TEMPLATE_EXT*)pvext; + const CRYPT_OID_INFO *tmpl_oid = find_oid(CRYPT_OID_INFO_NAME_KEY, tmpl_name, CRYPT_TEMPLATE_OID_GROUP_ID, true); + if (tmpl_oid && !stricmp(tmpl_oid->pszOID, cte->pszObjId)) + { + /* found OID match in extension against resolved key */ + gc_free(&gc); + return true; + } + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId)) + { + /* found direct OID match with certificate property specified */ + gc_free(&gc); + return true; + } + } + } + + /* no extension found, exit */ + gc_free(&gc); + return false; +} + static const CERT_CONTEXT * find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) { @@ -743,17 +852,25 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) * The first matching certificate that has not expired is returned. */ const CERT_CONTEXT *rv = NULL; - DWORD find_type; - const void *find_param; + DWORD find_type = CERT_FIND_ANY; + const void *find_param = NULL; + bool (*find_test)(const char*, const CERT_CONTEXT*) = NULL; unsigned char hash[255]; CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash}; struct gc_arena gc = gc_new(); - if (!strncmp(cert_prop, "SUBJ:", 5)) + if (!strncmp(cert_prop, "TMPL:", 5)) + { + /* skip the tag */ + cert_prop += 5; + find_test = &test_certificate_template; + } + else if (!strncmp(cert_prop, "SUBJ:", 5)) { /* skip the tag */ - find_param = wide_string(cert_prop + 5, &gc); + cert_prop += 5; find_type = CERT_FIND_SUBJECT_STR_W; + find_param = wide_string(cert_prop, &gc); } else if (!strncmp(cert_prop, "THUMB:", 6)) { @@ -819,12 +936,23 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) { validity = CertVerifyTimeValidity(NULL, rv->pCertInfo); } - if (!rv || validity == 0) + else { break; } - msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store %s.", - validity < 0 ? "not yet valid" : "that has expired"); + + if (validity == 0) + { + if (!find_test || find_test(cert_prop, rv)) + { + break; + } + } + else + { + msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store %s.", + validity < 0 ? "not yet valid" : "that has expired"); + } } out:
The certificate selection process for the Crypto API certificates is currently fixed to match on subject or identifier. Especially if certificates that are used for OpenVPN are managed by a Windows CA, it is appropriate to select the certificate to use by the template that it is generated from, especially on domain-joined clients which automatically acquire/renew the corresponding certificate. The attached match implements the match on TMPL: with either a template name (which is looked up through CryptFindOIDInfo) or by specifying the OID of the template directly, which then is matched against the corresponding X509 extensions specifying the template that the certificate was generated from. The logic requires to walk all certificates in the underlying store and to match the certificate extensions directly. The hook which is implemented in the certificate selection logic is generic to allow other Crypto-API certificate matches to also be implemented at some point in the future. The logic to match the certificate template is taken from the implementation in the .NET core runtime, see Pal.Windows/FindPal.cs in in the implementation of System.Security.Cryptography.X509Certificates. Signed-off-by: Heiko Wundram <heiko.wundram@gehrkens.it> --- src/openvpn/cryptoapi.c | 142 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 135 insertions(+), 7 deletions(-)