| Message ID | 1581519967-16950-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v4,1/2] Skip expired certificates in Windows certificate store | expand |
Built and tested with MSVC on Windows 10 - code skips expired certificate
and picks valid one.
Tested that code doesn't explode if --cryptoapicert has unsupported value.
Acked-by: Lev Stipakov <lstipakov@gmail.com>
<div dir="ltr"><div dir="ltr">Built and tested with MSVC on Windows 10 - code skips expired certificate and picks valid one.<div><br></div><div>Tested that code doesn't explode if --cryptoapicert has unsupported value.</div><div><br></div><div>Acked-by: Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>></div></div></div>
Your patch has been applied to the master branch.
Haven't done any real testing, just test build on MinGW (to have "the
other build environment"). Passes :-)
commit 7b63984d51a2582ba2d406e46a7debb11df7f478
Author: Selva Nair
Date: Wed Feb 12 10:06:06 2020 -0500
Skip expired certificates in Windows certificate store
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
Message-Id: <1581519967-16950-1-git-send-email-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19404.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, is this one and aa6affe6df811db11577847366a569def0a3e314 also material for release/2.4? So "feature" or "bug" category? (I've left this sit in my inbox and also in patchwork to remind me that I wanted to clarify this, and then never got around to actually do so) gert On Thu, Feb 13, 2020 at 08:54:18PM +0100, Gert Doering wrote: > Your patch has been applied to the master branch. > > Haven't done any real testing, just test build on MinGW (to have "the > other build environment"). Passes :-) > > commit 7b63984d51a2582ba2d406e46a7debb11df7f478 > Author: Selva Nair > Date: Wed Feb 12 10:06:06 2020 -0500 > > Skip expired certificates in Windows certificate store > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > Acked-by: Lev Stipakov <lstipakov@gmail.com> > Message-Id: <1581519967-16950-1-git-send-email-selva.nair@gmail.com> > URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19404.html > Signed-off-by: Gert Doering <gert@greenie.muc.de> > > > -- > kind regards, > > Gert Doering > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel >
Hi, > is this one and aa6affe6df811db11577847366a569def0a3e314 also material > for release/2.4? So "feature" or "bug" category? Yes it would be good to get this one and aa6affe into 2.4. This one will cherry-pick with a minor conflict in cryptoapicert.c, easily resolved. aa6affe should cherry-pick with no issues. Selva
Hi, On Wed, Apr 15, 2020 at 02:22:15PM -0400, Selva Nair wrote: > > is this one and aa6affe6df811db11577847366a569def0a3e314 also material > > for release/2.4? So "feature" or "bug" category? > > Yes it would be good to get this one and aa6affe into 2.4. This one > will cherry-pick with a minor conflict in cryptoapicert.c, easily > resolved. aa6affe should cherry-pick with no issues. That conflict was trivial indeed. I fixed an occurrance of "else {" while at it (whitespace only). Test built on Ubuntu/MinGW, not actually tested the resulting binary. Cursory review of the code also does not reveal anything unexpected. commit 5f8a9df1eea33c2d6fc267e5e3683449954c986b (HEAD -> release/2.4) Author: Selva Nair <selva.nair@gmail.com> Date: Wed Feb 12 10:06:07 2020 -0500 Allow unicode search string in --cryptoapicert option (cherry picked from commit aa6affe6df811db11577847366a569def0a3e314) commit 4658b3b6f6008eea1819ea26a46fd46df87b1030 Author: Selva Nair <selva.nair@gmail.com> Date: Wed Feb 12 10:06:06 2020 -0500 Skip expired certificates in Windows certificate store (cherry picked from commit 7b63984d51a2582ba2d406e46a7debb11df7f478) gert
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 2f2eee7..b9f1328 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -739,27 +739,30 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) * SUBJ:<certificate substring to match> * THUMB:<certificate thumbprint hex value>, e.g. * THUMB:f6 49 24 41 01 b4 fb 44 0c ce f4 36 ae d0 c4 c9 df 7a b6 28 + * The first matching certificate that has not expired is returned. */ const CERT_CONTEXT *rv = NULL; + DWORD find_type; + const void *find_param; + unsigned char hash[255]; + CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash}; if (!strncmp(cert_prop, "SUBJ:", 5)) { /* skip the tag */ - cert_prop += 5; - rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, CERT_FIND_SUBJECT_STR_A, cert_prop, NULL); - + find_param = cert_prop + 5; + find_type = CERT_FIND_SUBJECT_STR_A; } else if (!strncmp(cert_prop, "THUMB:", 6)) { - unsigned char hash[255]; - char *p; + const char *p; int i, x = 0; - CRYPT_HASH_BLOB blob; + find_type = CERT_FIND_HASH; + find_param = &blob; /* skip the tag */ cert_prop += 6; - for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++) + for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++) { if (*p >= '0' && *p <= '9') { @@ -775,7 +778,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) } if (!*++p) /* unexpected end of string */ { - break; + msg(M_WARN, "WARNING: cryptoapicert: error parsing <THUMB:%s>.", cert_prop); + return NULL; } if (*p >= '0' && *p <= '9') { @@ -796,10 +800,28 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) } } blob.cbData = i; - blob.pbData = (unsigned char *) &hash; - rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, CERT_FIND_HASH, &blob, NULL); + } + else { + msg(M_WARN, "WARNING: cryptoapicert: unsupported certificate specification <%s>", cert_prop); + return NULL; + } + while(true) + { + int validity = 1; + /* this frees previous rv, if not NULL */ + rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + 0, find_type, find_param, rv); + if (rv) + { + validity = CertVerifyTimeValidity(NULL, rv->pCertInfo); + } + if (!rv || validity == 0) + { + break; + } + msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store %s.", + validity < 0 ? "not yet valid" : "that has expired"); } return rv;