diff mbox series

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

Message ID 1581359742-30511-1-git-send-email-selva.nair@gmail.com
State Superseded
Headers show
Series [Openvpn-devel,1/2,v3] Skip expired certificates in Windows certificate store | expand

Commit Message

Selva Nair Feb. 10, 2020, 7:35 a.m. UTC 
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

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
v3: nudging again with a rebase to master

 src/openvpn/cryptoapi.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Lev Stipakov Feb. 10, 2020, 10:52 p.m. UTC | #1 
Hi,

+    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;
>      }
>      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);
>

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];
> +    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
<div dir="ltr"><div dir="ltr">Hi,</div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+    DWORD find_type;<br>+    const void *find_param;  </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">  </blockquote><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">     if (!strncmp(cert_prop, &quot;SUBJ:&quot;, 5))<br>
     { </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+        find_param = cert_prop + 5;<br>
+        find_type = CERT_FIND_SUBJECT_STR_A;<br>
     }<br>
     else if (!strncmp(cert_prop, &quot;THUMB:&quot;, 6))<br>
     {<br>+        find_type = CERT_FIND_HASH;<br>
+        find_param = &amp;blob;<br>+    }<br>
+    while(true)<br>
+    {<br>         rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,<br>
+                                        0, find_type, find_param, rv);<br></blockquote><div><br></div><div>This explodes if cert_prop doesn&#39;t start with either &quot;SUBJ:&quot; or &quot;THUMB:&quot; since we pass</div><div>uninitialized arguments.</div><div><br></div><div>This problem didn&#39;t exist before, since we looked for certificate inside above &quot;if&quot; blocks where</div><div>both variables are initialized.</div><div><br></div><div>Another thing:</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+    unsigned char hash[255];<br>+    CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};<br> <br>     else if (!strncmp(cert_prop, &quot;THUMB:&quot;, 6))<br>     {<br>-        unsigned char hash[255];<br>-        CRYPT_HASH_BLOB blob;<br></blockquote><div><br></div><div>Why did you move &quot;hash&quot; and &quot;blob&quot; to the outer scope? I think those</div><div>variables should stay where they have been, since they&#39;re not used outside of &quot;if&quot;.</div><div><br></div><div>-Lev</div></div></div>
Selva Nair Feb. 11, 2020, 5:23 a.m. UTC | #2 
Hi,

Thanks for reviewing this.

On Tue, Feb 11, 2020 at 4:52 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> Hi,
>
>> +    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;
>>      }
>>      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);
>
>
> 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.

>
> 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];
>> +    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".

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
diff mbox series

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 2f2eee7..3b70c33 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,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;