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