[Openvpn-devel,v4,1/2] Skip expired certificates in Windows certificate store

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
Related show

Commit Message

Selva Nair Feb. 12, 2020, 3:06 p.m.
From: Selva Nair <selva.nair@gmail.com>

Have the cryptoapicert option find the first matching certificate
in store that is valid at the present time. Currently the first
found item, even if expired, is returned.

This makes it possible to update certifiates in store without having
to delete old ones. As a side effect, if only expired certificates are
found, the connection fails.

Also remove some unnecessary casts.

Tested on Windows 10.
Trac #966

v4: Handle the case when an unknown certificate specification is passed
to find_certificate_in_store().

Note: Warnings printed from find_certificate_in_store() could show up
multiple times as its called for each certificate store. This could
be improved in a future patch.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/cryptoapi.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

Comments

Lev Stipakov Feb. 13, 2020, 8:37 a.m. | #1
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&#39;t explode if --cryptoapicert has unsupported value.</div><div><br></div><div>Acked-by: Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt;</div></div></div>
Gert Doering Feb. 13, 2020, 7:54 p.m. | #2
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
Gert Doering April 15, 2020, 5:58 p.m. | #3
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
>
Selva Nair April 15, 2020, 6:22 p.m. | #4
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
Gert Doering April 15, 2020, 7:12 p.m. | #5
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

Patch

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;