diff mbox series

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

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

Commit Message

Selva Nair March 11, 2018, 2:17 p.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>
---
 src/openvpn/cryptoapi.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

Comments

Gert Doering March 11, 2018, 9:21 p.m. UTC | #1 
Hi Selva,

On Sun, Mar 11, 2018 at 09:17:58PM -0400, selva.nair@gmail.com wrote:
> 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.

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
Selva Nair March 12, 2018, 3:37 a.m. UTC | #2 
Hi,

On Mon, Mar 12, 2018 at 4:21 AM, Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi Selva,
>
> On Sun, Mar 11, 2018 at 09:17:58PM -0400, selva.nair@gmail.com wrote:
> > 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.
>
> 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 :-)


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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering March 12, 2018, 5:15 a.m. UTC | #3 
Hi,

On Mon, Mar 12, 2018 at 10:37:53AM -0400, Selva Nair wrote:
> 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.

> 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
Steffan Karger April 2, 2018, 2:37 a.m. UTC | #4 
Hi,

One comment based on stare-at-code only:

On 12-03-18 02:17, selva.nair@gmail.com wrote:
> @@ -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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair April 2, 2018, 4:58 a.m. UTC | #5 
Hi,

Thanks for looking at this.

On Mon, Apr 2, 2018 at 8:37 AM, Steffan Karger <steffan@karger.me> wrote:
>
> Hi,
>
> One comment based on stare-at-code only:
>
> On 12-03-18 02:17, selva.nair@gmail.com wrote:
> > @@ -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.

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Steffan Karger April 2, 2018, 6:04 a.m. UTC | #6 
Hi,

On 02-04-18 16:58, Selva Nair wrote:
> On Mon, Apr 2, 2018 at 8:37 AM, Steffan Karger <steffan@karger.me> wrote:
>> 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..

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

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox series

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 11b971f..a579854 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -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;