| Message ID | 20210525092510.27517-1-heiko.wundram@gehrkens.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Implement Windows CA template match for Crypto-API selector | expand |
Hi, I'm not convinced of the utility of this. It could be marginally useful in some setups where all users are tied to a domain and choosing any certificate that matches a template is appropriate. I don't have a setup to test this. Here are some general comments anyway. On Tue, May 25, 2021 at 5:44 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 | 117 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 112 insertions(+), 5 deletions(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index ded8c914..ee4c9014 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -732,6 +732,105 @@ 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 cound for decoding */ typo: count > + 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) > + { Why not do the local lookup for all grupids? Anyway you are falling back to domain-wide look up if this fails. > + 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); How long would this take to time out -- we try to avoid calling functions that need network access to the DC as that's often not available before the tunnel comes up. > + } > + > + /* fall back to all groups if not found yet and fallback requested */ > + if (!info && fallback && groupid) > + { > + info = CryptFindOIDInfo(keytype, (void*)key, 0); That could be a second network call. Why not combine the two into one call. > + } > + > + return info; > +} > + > +static bool > +test_certificate_template(struct gc_arena *gc, const char *cert_prop, const CERT_CONTEXT *cert_ctx) > +{ As this is called from a loop for every certificate in the store until a match is found, passing in gc that will be released only after exiting the loop is wasteful memory use. Just create a new gc clear and clear it at exit. You are not using any buffers allocated here after return. > + const CERT_INFO *info = cert_ctx->pCertInfo; > + const CERT_EXTENSION *ext; > + DWORD cbext; > + void *pvext; > + 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 (!_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name)) Is it guaranteed that pbData is NULL terminated? (MS docs are unclear on this). We may have to add a safety check that the data length length is >= wcslen(tmpl_name). > + { > + /* data content matches extension name */ > + 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 */ > + return true; > + } > + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId)) > + { Why not test this first so that the call to find_oid could be avoided if this matches? > + /* found direct OID match with certificate property specified */ > + return true; > + } > + } > + } > + > + /* no extension found, exit */ > + return false; > +} > + > static const CERT_CONTEXT * > find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > { > @@ -743,17 +842,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)(struct gc_arena*, 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,7 +926,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > { > validity = CertVerifyTimeValidity(NULL, rv->pCertInfo); > } > - if (!rv || validity == 0) > + if (!rv || (validity == 0 && (!find_test || find_test(&gc, cert_prop, rv)))) > { > break; > } The warning log below this is meant to be printed only when the cert matches but validity period is off. But now it will print a warning for every certificate in the store until a match is found. That's not acceptable. There are some missing {} for if/else, we prefer && at the start of line for multi-line conditions etc. See https://community.openvpn.net/openvpn/wiki/CodeStyle Selva
Hello Selva, I'll send an updated patch wrt. some of your notes, for now just a quick reply to some of them: > I'm not convinced of the utility of this. It could be marginally useful in some > setups where all users are tied to a domain and choosing any certificate that > matches a template is appropriate. I don't have a setup to test this. That's not the only use case. I'm currently deploying this patch to implement a machine always-on VPN (machines automatically register a specific certificate type which contains the machine name as a SAN, which can then be selected using the TMPL: logic and on the server side the client certificate looked up in a tls-verify script to check whether the machine account is active). This patch allows moving the client certificate selection out of the configuration file, as the selector based on template ID is always equal on any client (SUBJ of course isn't) and as such can be embedded as a plain .ovpn file in the MSI which deploys the software, without any additional configuration required on the part of the client, and especially resilient to later renaming of the client if that should happen (new certificate deployed with certutil /pulse, new VPN connection found). Same of course goes for your scenario of user auto-registered certificates. > > +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) > > + { > > Why not do the local lookup for all grupids? Anyway you are falling back to > domain-wide look up if this fails. Simply because Microsoft does it this way in their generic wrapping of CryptFindOIDInfo for certificate selection helpers: https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/Interop/Windows/Crypt32/Interop.FindOidInfo.cs#L51 ff - the groups excluded here will always be looked up locally, independent of the CRYPT_OID_DISABLE_SEARCH_FLAG, so that we can save one call for them. > > + 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); > > How long would this take to time out -- we try to avoid calling functions that > need network access to the DC as that's often not available before the tunnel > comes up. Good question which I hadn't taken into account so far; from what I've seen on machine clients I've deployed that select their certificate using this logic, the timeout is almost instantaneous, i.e. CryptFindOIDInfo does not find an object for the TMPL: parameter if it can't reach the domain. Of course, your note generally limits the functionality of actually using CryptFindOIDInfo to resolve a printable template name from a specific domain to an OID (which is then found in the actual certificate), but I wouldn't take this out if it doesn't introduce a long delay (possibly, there are two tunnels in some cases, where an independent tunnel sets up reachability of the domain). > > + } > > + > > + /* fall back to all groups if not found yet and fallback requested */ > > + if (!info && fallback && groupid) > > + { > > + info = CryptFindOIDInfo(keytype, (void*)key, 0); > > That could be a second network call. Why not combine the two into one call. The difference being that the first call only resolves the name to OID for the specific object type (e.g., certificate template) being passed, whereas this call resolves it for any object type. Of course, you always prefer the specific match if that's available, and only fall back to the generic match if the caller actually wants to try that. I've implemented this method more or less equal to the implementation in the .NET framework, simply to facilitate using it for additional certificate matches which might want to be implemented; there are other use cases for CryptFindOIDInfo in other match helpers where this logic actually makes sense. It's debatable whether the fallback to a generic match actually makes sense in the case of certificate template OID matching (see the use below), but at least the .NET framework implementation does this: https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L190 > > + /* 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 (!_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name)) > > Is it guaranteed that pbData is NULL terminated? (MS docs are unclear on this). > We may have to add a safety check that the data length length is >= > wcslen(tmpl_name). Yes, that's guaranteed, at least according to the corresponding use in the .NET framework: https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L165 - of course it doesn't hurt to have an additional check anyway. > > + 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 */ > > + return true; > > + } > > + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId)) > > + { > > Why not test this first so that the call to find_oid could be avoided if this > matches? For one, because the match in https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L187 ff also does it just this way, and secondly, because this way the preference is on matching a template name (i.e., if a string passed can be interpreted as a template name in the current domain, it will be, even if it looks like an OID), whereas only resolving when the OID doesn't match directly would prefer the interpretation as an OID. It's debatable whether the former or the latter is preferential, I'd also go to the former, just like the .NET framework does, especially considering that the "old" X509 extension containing the certificate template reference actually contains the name, not an OID (see the first condition), so that possibly some people actually expect the parameter to be a name. For all the other notes, those are clear; I'll create a new patch with the changes applied. Thanks for your feedback! --- Heiko.
Hi, > > > > +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) > > > + { > > > > Why not do the local lookup for all grupids? Anyway you are falling back to > > domain-wide look up if this fails. > > > Simply because Microsoft does it this way in their generic wrapping of > CryptFindOIDInfo for certificate selection helpers: > https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/Interop/Windows/Crypt32/Interop.FindOidInfo.cs#L51 > ff - the groups excluded here will always be looked up locally, independent > of the CRYPT_OID_DISABLE_SEARCH_FLAG, so that we can save one call for them. > > > > + info = CryptFindOIDInfo(keytype, (void*)key, groupid | > > CRYPT_OID_DISABLE_SEARCH_DS_FLAG); You are misreading the usage in dotnet sources. Their reasoning is that for those listed algorithms AD will not be consulted and there is no need to explicitly add the DISABLE_SEARCH_FLAG. But adding it doesn't hurt and simplifies the code. So change the above to info = CryptFindOIDInfo(keytype, (void*)key, groupid|CRYPT_OID_DISABLE_SEARCH_DS_FLAG); with no if clause. That way we first search without querying the AD irrespective of the algorithm. > > > + } > > > + > > > + /* try proper resolve if not found yet, also including AD */ > > > + if (!info) > > > + { > > > + info = CryptFindOIDInfo(keytype, (void*)key, groupid); > > > > How long would this take to time out -- we try to avoid calling functions that > > need network access to the DC as that's often not available before the tunnel > > comes up. > > Good question which I hadn't taken into account so far; from what I've seen on machine clients I've deployed that select their certificate using this logic, the timeout is almost instantaneous, i.e. CryptFindOIDInfo does not find an object for the TMPL: parameter if it can't reach the domain. Of course, your note generally limits the functionality of actually using CryptFindOIDInfo to resolve a printable template name from a specific domain to an OID (which is then found in the actual certificate), but I wouldn't take this out if it doesn't introduce a long delay (possibly, there are two tunnels in some cases, where an independent tunnel sets up reachability of the domain). Actually there are reports out there that CryptFindOIDInfo can take a long while to timeout if AD is unreachable. To make things worse we are here querying the AD two times one with groupid and second time for all groups. But we can't do much about it except to add a caveat that anyone using template match should be aware of it. Please also update the man page (description of --cryptoapicert). Selva <div dir="ltr">Hi,<div><br>><br>> > > +static const CRYPT_OID_INFO *<br>> > > +find_oid(DWORD keytype, const void *key, DWORD groupid, bool<br>> > > +fallback) {<br>> > > + const CRYPT_OID_INFO *info = NULL;<br>> > > +<br>> > > + /* force resolve from local as first step */<br>> > > + if (groupid != CRYPT_HASH_ALG_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_ENCRYPT_ALG_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_PUBKEY_ALG_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_SIGN_ALG_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_RDN_ATTR_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_EXT_OR_ATTR_OID_GROUP_ID &&<br>> > > + groupid != CRYPT_KDF_OID_GROUP_ID)<br>> > > + {<br>> ><br>> > Why not do the local lookup for all grupids? Anyway you are falling back to<br>> > domain-wide look up if this fails.<br>><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> Simply because Microsoft does it this way in their generic wrapping of CryptFindOIDInfo for certificate selection helpers: <a href="https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/Interop/Windows/Crypt32/Interop.FindOidInfo.cs#L51">https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/Interop/Windows/Crypt32/Interop.FindOidInfo.cs#L51</a> ff - the groups excluded here will always be looked up locally, independent of the CRYPT_OID_DISABLE_SEARCH_FLAG, so that we can save one call for them.</blockquote><br>><br>> > > + info = CryptFindOIDInfo(keytype, (void*)key, groupid |<br>> > CRYPT_OID_DISABLE_SEARCH_DS_FLAG);<br><br>You are misreading the usage in dotnet sources. Their reasoning is that for those listed algorithms AD will not be consulted and there is no need to explicitly add the DISABLE_SEARCH_FLAG. But adding it doesn't hurt and simplifies the code.<br><br>So change the above to<br><br><div>info = CryptFindOIDInfo(keytype, (void*)key, groupid|CRYPT_OID_DISABLE_SEARCH_DS_FLAG);<br><br>with no if clause. That way we first search without querying the AD irrespective of the algorithm.<div><br>> > > + }<br>> > > +<br>> > > + /* try proper resolve if not found yet, also including AD */<br>> > > + if (!info)<br>> > > + {<br>> > > + info = CryptFindOIDInfo(keytype, (void*)key, groupid);<br>> ><br>> > How long would this take to time out -- we try to avoid calling functions that<br>> > need network access to the DC as that's often not available before the tunnel<br>> > comes up.<br>><br>> Good question which I hadn't taken into account so far; from what I've seen on machine clients I've deployed that select their certificate using this logic, the timeout is almost instantaneous, i.e. CryptFindOIDInfo does not find an object for the TMPL: parameter if it can't reach the domain. Of course, your note generally limits the functionality of actually using CryptFindOIDInfo to resolve a printable template name from a specific domain to an OID (which is then found in the actual certificate), but I wouldn't take this out if it doesn't introduce a long delay (possibly, there are two tunnels in some cases, where an independent tunnel sets up reachability of the domain).</div><div><br></div><div>Actually there are reports out there that CryptFindOIDInfo can take a long while to timeout if AD is unreachable. To make things worse we are here querying the AD two times one with groupid and second time for all groups. But we can't do much about it except to add a caveat that anyone using template match should be aware of it. </div><div><br></div></div><div>Please also update the man page (description of --cryptoapicert).</div><div><br></div><div>Selva</div></div></div>
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index ded8c914..ee4c9014 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -732,6 +732,105 @@ 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 cound 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(struct gc_arena *gc, const char *cert_prop, const CERT_CONTEXT *cert_ctx) +{ + const CERT_INFO *info = cert_ctx->pCertInfo; + const CERT_EXTENSION *ext; + DWORD cbext; + void *pvext; + 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 (!_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name)) + { + /* data content matches extension name */ + 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 */ + return true; + } + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId)) + { + /* found direct OID match with certificate property specified */ + return true; + } + } + } + + /* no extension found, exit */ + return false; +} + static const CERT_CONTEXT * find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) { @@ -743,17 +842,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)(struct gc_arena*, 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,7 +926,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) { validity = CertVerifyTimeValidity(NULL, rv->pCertInfo); } - if (!rv || validity == 0) + if (!rv || (validity == 0 && (!find_test || find_test(&gc, cert_prop, rv)))) { break; }
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 | 117 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 112 insertions(+), 5 deletions(-)