Skip to contentSkip to conversation

Skip expired certificates in Windows certificate store

PATCH Superseded [email protected]... 4 participants 23 messages Mar 12, 2018 – Apr 15, 2020

lore
Share & tools
lore

[email protected]...

author
#0 lore raw

PATCH 1/2 Skip expired certificates in Windows certificate store

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/cryptoapi.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)
src/openvpn/cryptoapi.c +30 -11
index 11b971f..a579854 100644
@@ -601,27 +601,31 @@ 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')
             {
                 x = (*p - '0') << 4;

@@ -636,6 +640,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
             if (!*++p)  /* unexpected end of string */
             {
+                msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", cert_prop);
+                return NULL;
                 break;
             }
             if (*p >= '0' && *p <= '9')

@@ -657,10 +663,23 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
         }
         blob.cbData = i;
-        blob.pbData = (unsigned char *) &hash;
+    }
+    while(true)
+    {
+        int validity = 1;
+        /* this frees previous rv, if not NULL */
         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-                                        0, CERT_FIND_HASH, &blob, NULL);
-
+                                        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;
Signature · 1 line
2.1.4
author
#1 lore raw

PATCH 2/2 Allow unicode search string in --cryptoapicert option

From: Selva Nair <[email protected]...>

Currently when the certificate is specified as "SUBJ:foo", the string foo is assumed to be ascii. Change that and interpret it as utf-8, convert to a wide string, and flag it as unicode in CertFindCertifcateInStore().

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/cryptoapi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
src/openvpn/cryptoapi.c +7 -4
index a579854..ae4a01b 100644
@@ -50,6 +50,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.

@@ -608,12 +609,13 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     const void *find_param;
     unsigned char hash[255];
     CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+    struct gc_arena gc = gc_new();
 
     if (!strncmp(cert_prop, "SUBJ:", 5))
     {
         /* skip the tag */
-        find_param = cert_prop + 5;
-        find_type = CERT_FIND_SUBJECT_STR_A;
+        find_param = wide_string(cert_prop + 5, &gc);
+        find_type = CERT_FIND_SUBJECT_STR_W;
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {

@@ -641,8 +643,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             if (!*++p)  /* unexpected end of string */
             {
                 msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", cert_prop);
-                return NULL;
-                break;
+                goto out;
             }
             if (*p >= '0' && *p <= '9')
             {

@@ -682,6 +683,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             validity < 0 ? "not yet valid" : "that has expired");
     }
 
+out:
+    gc_free(&gc);
     return rv;
 }
Signature · 1 line
2.1.4
#2 lore raw
In reply to [email protected]...

Hi Selva,

Quoted 5 lines · On Sun, Mar 11, 2018 at 09:17:58PM -0400, [email protected]... wrote:

From: Selva Nair <[email protected]...>

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.

Are these two intended for master only or master+2.4?

(I admit that I am too lazy right now to go and actually look at the surrounding code :-) - but with all the recent work wrt cryptoapi and external management key, I lost track which bits are considered "new goodies for master only")

Functionality-wise this makes sense (feature-ACK), and it also makes sense for 2.4 - because "if there are two certificates, an expired and a valid one, and we take the expired one" smells very much like a bug to me :-)

gert

Signature · 3 lines
now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...
author
#3 lore raw

Hi,

Quoted 17 lines · Gert Doering

Hi Selva,

Quoted 5 lines · On Sun, Mar 11, 2018 at 09:17:58PM -0400, [email protected]... wrote:

From: Selva Nair <[email protected]...>

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.

Are these two intended for master only or master+2.4?

(I admit that I am too lazy right now to go and actually look at the surrounding code :-) - but with all the recent work wrt cryptoapi and external management key, I lost track which bits are considered "new goodies for master only")

Quoted 4 lines

Functionality-wise this makes sense (feature-ACK), and it also makes sense for 2.4 - because "if there are two certificates, an expired and a valid one, and we take the expired one" smells very much like a bug to me :-)

Agree, this could qualify for 2.4. Anyway, the context is the same and it applies/cherry-picks to 2.4 without issues.

Elsewhere in the code we only warn about expired certs (like those read from a file) and continue with the connection to eventually end up with the unhelpful "TLS key negotiation failed to complete. Check your network..." error [*]. In that sense this is a regression. IMO, the client should error out on invalid certs from other sources as well.

Selva

#4 lore raw

Hi,

Quoted 2 lines · Selva Nair

Agree, this could qualify for 2.4. Anyway, the context is the same and it applies/cherry-picks to 2.4 without issues.

OK, thanks.

Quoted 5 lines

Elsewhere in the code we only warn about expired certs (like those read from a file) and continue with the connection to eventually end up with the unhelpful "TLS key negotiation failed to complete. Check your network..." error [*]. In that sense this is a regression. IMO, the client should error out on invalid certs from other sources as well.

Well. In other cases, we do not have anything else to try, and when Steffan added the "oh, this might have expired!" warning, it was much better than the "mysterious TLS handshake failure" we had before.

The reason it's only a warning was a conscious decision: our clock might be wrong, or sufficiently different from the server's clock that it actually *could* be valid on his side. So we print a warning so it's more obvious why it fails, but try anyway.

But I do not have strong feelings either way, so if "you" (for an unspecified value of "you") want to change this to error out, fine with me :-) - I just wanted to provide background.

gert

Signature · 3 lines
now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...
#5 lore raw
In reply to [email protected]...

Hi,

One comment based on stare-at-code only:

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 646

Quoted 8 lines · On 12-03-18 02:17, [email protected]... wrote:
src/openvpn/cryptoapi.c +2 -0
@@ -636,6 +640,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
             if (!*++p)  /* unexpected end of string */
             {
+                msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", cert_prop);
+                return NULL;
                 break;
             }

The break after the return looks a bit strange, maybe remove it?

Also, this looks like a somewhat unrelated fix. I would have personally preferred it in a separate patch (so we can e.g. backport it easily even if we decide not not backport the functional change).

Otherwise the code and functional changes look good to me. I'll try to find some time to compile this and test it on windows later.

-Steffan

author
#6 lore raw

Hi,

Thanks for looking at this.

Quoted 6 lines · Steffan Karger

Hi,

One comment based on stare-at-code only:

On 12-03-18 02:17, [email protected]... wrote:

Quoted 10 lines

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 646

Quoted 8 lines
src/openvpn/cryptoapi.c +2 -0
@@ -636,6 +640,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
             if (!*++p)  /* unexpected end of string */
             {
+                msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", cert_prop);
+                return NULL;
                 break;
             }

The break after the return looks a bit strange, maybe remove it?

Agreed, the break is dead code and should go. Will send a v2.

Quoted 5 lines

Also, this looks like a somewhat unrelated fix. I would have personally preferred it in a separate patch (so we can e.g. backport it easily even if we decide not not backport the functional change).

The original did the search based on subject and hash separately, which is now combined into a loop below this parsing chunk. So a return, in place of break, is required here. The warning on parse error is new (instead of silently returning NULL). But the end result of that is still a FATAL error later in the code, as before.

I'm ok to leave out the warning from this patch, though..

Selva

#7 lore raw

Hi,

Quoted 5 lines · Selva Nair
Quoted 3 lines · Steffan Karger

Also, this looks like a somewhat unrelated fix. I would have personally preferred it in a separate patch (so we can e.g. backport it easily even if we decide not not backport the functional change).

Quoted 7 lines

The original did the search based on subject and hash separately, which is now combined into a loop below this parsing chunk. So a return, in place of break, is required here. The warning on parse error is new (instead of silently returning NULL). But the end result of that is still a FATAL error later in the code, as before.

I'm ok to leave out the warning from this patch, though..

Ah, ok. If it's not independent, let's just keep it in this patch.

-Steffan

author
#8 lore raw

PATCH v2 1/2 Skip expired certificates in Windows certificate store

In reply to [email protected]...

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>

---
v2: remove the break after return

 src/openvpn/cryptoapi.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)
src/openvpn/cryptoapi.c +30 -12
index 11b971f..ec7569a 100644
@@ -601,27 +601,31 @@ 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')
             {
                 x = (*p - '0') << 4;

@@ -636,7 +640,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')
             {

@@ -657,10 +662,23 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
         }
         blob.cbData = i;
-        blob.pbData = (unsigned char *) &hash;
+    }
+    while(true)
+    {
+        int validity = 1;
+        /* this frees previous rv, if not NULL */
         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-                                        0, CERT_FIND_HASH, &blob, NULL);
-
+                                        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;
Signature · 1 line
2.1.4
author
#9 lore raw

PATCH v2 2/2 Allow unicode search string in --cryptoapicert option

From: Selva Nair <[email protected]...>

Currently when the certificate is specified as "SUBJ:foo", the string foo is assumed to be ascii. Change that and interpret it as utf-8, convert to a wide string, and flag it as unicode in CertFindCertifcateInStore().

Signed-off-by Selva Nair <[email protected]...>

---
v2: rebased to v2 1/2 -- no code changes

 src/openvpn/cryptoapi.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
src/openvpn/cryptoapi.c +7 -3
index ec7569a..c78e608 100644
@@ -50,6 +50,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.

@@ -608,12 +609,13 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     const void *find_param;
     unsigned char hash[255];
     CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+    struct gc_arena gc = gc_new();
 
     if (!strncmp(cert_prop, "SUBJ:", 5))
     {
         /* skip the tag */
-        find_param = cert_prop + 5;
-        find_type = CERT_FIND_SUBJECT_STR_A;
+        find_param = wide_string(cert_prop + 5, &gc);
+        find_type = CERT_FIND_SUBJECT_STR_W;
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {

@@ -641,7 +643,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             if (!*++p)  /* unexpected end of string */
             {
                 msg(M_WARN, "WARNING: cryptoapicert: error parsing <THUMB:%s>.", cert_prop);
-                return NULL;
+                goto out;
             }
             if (*p >= '0' && *p <= '9')
             {

@@ -681,6 +683,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             validity < 0 ? "not yet valid" : "that has expired");
     }
 
+out:
+    gc_free(&gc);
     return rv;
 }
Signature · 1 line
2.1.4
author
#10 lore raw

PATCH v3 1/2 Skip expired certificates in Windows certificate store

In reply to [email protected]...

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>

---
v3: nudging again with a rebase to master

 src/openvpn/cryptoapi.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)
src/openvpn/cryptoapi.c +29 -12
index 2f2eee7..3b70c33 100644
@@ -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,23 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             }
         }
         blob.cbData = i;
-        blob.pbData = (unsigned char *) &hash;
+    }
+    while(true)
+    {
+        int validity = 1;
+        /* this frees previous rv, if not NULL */
         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-                                        0, CERT_FIND_HASH, &blob, NULL);
-
+                                        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;
Signature · 1 line
2.1.4
author
#11 lore raw

PATCH v3 2/2 Allow unicode search string in --cryptoapicert option

From: Selva Nair <[email protected]...>

Currently when the certificate is specified as "SUBJ:foo", the string foo is assumed to be ascii. Change that and interpret it as utf-8, convert to a wide string, and flag it as unicode in CertFindCertifcateInStore().

Signed-off-by Selva Nair <[email protected]...>

---
v3: nudging again, with a rebase to master

 src/openvpn/cryptoapi.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
src/openvpn/cryptoapi.c +7 -3
index 3b70c33..acae96f 100644
@@ -51,6 +51,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.

@@ -746,12 +747,13 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     const void *find_param;
     unsigned char hash[255];
     CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+    struct gc_arena gc = gc_new();
 
     if (!strncmp(cert_prop, "SUBJ:", 5))
     {
         /* skip the tag */
-        find_param = cert_prop + 5;
-        find_type = CERT_FIND_SUBJECT_STR_A;
+        find_param = wide_string(cert_prop + 5, &gc);
+        find_type = CERT_FIND_SUBJECT_STR_W;
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {

@@ -779,7 +781,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             if (!*++p)  /* unexpected end of string */
             {
                 msg(M_WARN, "WARNING: cryptoapicert: error parsing <THUMB:%s>.", cert_prop);
-                return NULL;
+                goto out;
             }
             if (*p >= '0' && *p <= '9')
             {

@@ -819,6 +821,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             validity < 0 ? "not yet valid" : "that has expired");
     }
 
+out:
+    gc_free(&gc);
     return rv;
 }
Signature · 1 line
2.1.4
#12 lore raw
In reply to [email protected]...

Hi,

+ DWORD find_type;

Quoted 1 line

+ const void *find_param;

     if (!strncmp(cert_prop, "SUBJ:", 5))
Quoted 1 line
     {

+ find_param = cert_prop + 5;

Quoted 10 lines
src/openvpn/cryptoapi.c +6 -0
+        find_type = CERT_FIND_SUBJECT_STR_A;
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {
+        find_type = CERT_FIND_HASH;
+        find_param = &blob;
+    }
+    while(true)
+    {
         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING |
Quoted 3 lines

PKCS_7_ASN_ENCODING,
+ 0, find_type, find_param, rv);

This explodes if cert_prop doesn't start with either "SUBJ:" or "THUMB:" since we pass
uninitialized arguments.

This problem didn't exist before, since we looked for certificate inside above "if" blocks where
both variables are initialized.

Another thing:

+ unsigned char hash[255];

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 758

Quoted 7 lines
src/openvpn/cryptoapi.c +1 -2
+    CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
 
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {
-        unsigned char hash[255];
-        CRYPT_HASH_BLOB blob;

Why did you move "hash" and "blob" to the outer scope? I think those variables should stay where they have been, since they're not used outside of "if".

-Lev

author
#13 lore raw

Hi,

Thanks for reviewing this.

Quoted 25 lines · Lev Stipakov

Hi,

Quoted 10 lines
src/openvpn/cryptoapi.c +4 -0
+    DWORD find_type;
+    const void *find_param;
 
 
 
     if (!strncmp(cert_prop, "SUBJ:", 5))
     {
 
+        find_param = cert_prop + 5;
+        find_type = CERT_FIND_SUBJECT_STR_A;

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)

Quoted 10 lines
src/openvpn/cryptoapi.c +6 -0
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {
+        find_type = CERT_FIND_HASH;
+        find_param = &blob;
+    }
+    while(true)
+    {
         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
+                                        0, find_type, find_param, rv);
Quoted 2 lines

This explodes if cert_prop doesn't start with either "SUBJ:" or "THUMB:" since we pass uninitialized arguments.

That's a terrible oversight. My bad. v4 is coming.

Quoted 9 lines

This problem didn't exist before, since we looked for certificate inside above "if" blocks where both variables are initialized.

Another thing:

Quoted 3 lines
src/openvpn/cryptoapi.c +2 -0
+    unsigned char hash[255];
+    CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
Quoted 8 lines

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 758

Quoted 4 lines
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {
src/openvpn/cryptoapi.c +0 -2
-        unsigned char hash[255];
-        CRYPT_HASH_BLOB blob;

Why did you move "hash" and "blob" to the outer scope? I think those variables should stay where they have been, since they're not used outside of "if".

The actual certificate search is now done outside (in the while loop) and it refers
to find_param which could be &blob.

Alternatively one could do the search separately for SUBJ and THUMB as in the original,
but with the new logic of iterating through the store, a combined approach looked
"cleaner".

Selva

author
#14 lore raw

PATCH v4 1/2 Skip expired certificates in Windows certificate store

In reply to [email protected]...

From: Selva Nair <[email protected]...>

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 <[email protected]...>
---
 src/openvpn/cryptoapi.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)
src/openvpn/cryptoapi.c +34 -12
index 2f2eee7..b9f1328 100644
@@ -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;
Signature · 1 line
2.1.4
author
#15 lore raw

PATCH v4 2/2 Allow unicode search string in --cryptoapicert option

From: Selva Nair <[email protected]...>

Currently when the certificate is specified as "SUBJ:foo", the string foo is assumed to be ascii. Change that and interpret it as utf-8, convert to a wide string, and flag it as unicode in CertFindCertifcateInStore().

Signed-off-by Selva Nair <[email protected]...>

---
v4: matched to v4 of 1/2

 src/openvpn/cryptoapi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
src/openvpn/cryptoapi.c +8 -4
index b9f1328..1bf74fc 100644
@@ -51,6 +51,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.

@@ -746,12 +747,13 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     const void *find_param;
     unsigned char hash[255];
     CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+    struct gc_arena gc = gc_new();
 
     if (!strncmp(cert_prop, "SUBJ:", 5))
     {
         /* skip the tag */
-        find_param = cert_prop + 5;
-        find_type = CERT_FIND_SUBJECT_STR_A;
+        find_param = wide_string(cert_prop + 5, &gc);
+        find_type = CERT_FIND_SUBJECT_STR_W;
     }
     else if (!strncmp(cert_prop, "THUMB:", 6))
     {

@@ -779,7 +781,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             if (!*++p)  /* unexpected end of string */
             {
                 msg(M_WARN, "WARNING: cryptoapicert: error parsing <THUMB:%s>.", cert_prop);
-                return NULL;
+                goto out;
             }
             if (*p >= '0' && *p <= '9')
             {

@@ -803,7 +805,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     }
     else {
         msg(M_WARN, "WARNING: cryptoapicert: unsupported certificate specification <%s>", cert_prop);
-        return NULL;
+        goto out;
     }
 
     while(true)

@@ -824,6 +826,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
             validity < 0 ? "not yet valid" : "that has expired");
     }
 
+out:
+    gc_free(&gc);
     return rv;
 }
Signature · 1 line
2.1.4
#17 lore raw

[Openvpn-devel] [PATCH applied] Re: Allow unicode search string in --cryptoapicert option

In reply to [email protected]...

Your patch has been applied to the master branch.

Same as for ther other patch, no review, just MinGW test build (passes).

commit aa6affe6df811db11577847366a569def0a3e314

Author: Selva Nair
Date:   Wed Feb 12 10:06:07 2020 -0500
     Allow unicode search string in --cryptoapicert option

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Lev Stipakov <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg19405.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#19 lore raw
In reply to [email protected]...

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 <[email protected]...>
     Acked-by: Lev Stipakov <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg19404.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#20 lore raw

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

Quoted 30 lines · Gert Doering

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 <[email protected]...>
     Acked-by: Lev Stipakov <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg19404.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...
author
#21 lore raw

Hi,

Quoted 2 lines

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

#22 lore raw

Hi,

Quoted 6 lines · Selva Nair
Quoted 2 lines

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 <[email protected]...>
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 <[email protected]...>
Date:   Wed Feb 12 10:06:06 2020 -0500

Skip expired certificates in Windows certificate store (cherry picked from commit 7b63984d51a2582ba2d406e46a7debb11df7f478)

gert

Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...