[Openvpn-devel] [PATCH v2 2/2] Allow unicode search string in --cryptoapicert option

[selva . nair]

From: Selva Nair 

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 
---
v2: rebased to v2 1/2 -- no code changes

 src/openvpn/cryptoapi.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index ec7569a..c78e608 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -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, );
+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 
.", 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();
 return rv;
 }
 
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[selva . nair]

From: Selva Nair 

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 
---
v2: remove the break after return 

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

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 11b971f..ec7569a 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:
  * THUMB:, 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 = 
 
 /* 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 
.", 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 *) 
+}
+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, , 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;
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v3] Improve management-external-key/cert error handling

[Selva Nair]

Hi,

This one applies cleanly on top of master.

On Mon, Apr 2, 2018 at 7:44 AM, Steffan Karger  wrote:
>
> Check the return values of management_query_cert() and
> tls_ctx_use_external_private_key(), and error out with a more descriptive
> error message.  To do so, we make the openssl-backed implementation of
> tls_ctx_use_external_private_key() not throw fatal error anymore.
>
> (And fix line wrapping while touching this code.)
>
> Signed-off-by: Steffan Karger 
> ---
> v2: error out with M_FATAL as suggested by Selva.
> v3: rebase on master (without extra patches)
>
>  src/openvpn/ssl.c | 28 
>  src/openvpn/ssl_openssl.c |  2 +-
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 669f941b..b06820ba 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -660,18 +660,30 @@ init_ssl(const struct options *options, struct 
> tls_root_ctx *new_ctx)
>  else if ((options->management_flags & MF_EXTERNAL_KEY)
>   && (options->cert_file || options->management_flags & 
> MF_EXTERNAL_CERT))
>  {
> -if (options->cert_file)
> +if (options->cert_file
> +&& 0 != tls_ctx_use_external_private_key(new_ctx,
> + options->cert_file,
> + 
> options->cert_file_inline))
>  {
> -tls_ctx_use_external_private_key(new_ctx, options->cert_file,
> - options->cert_file_inline);
> +msg(M_FATAL, "Failed to initialize management-external-key");
>  }
>  else

But I can't believe I missed this in the last round. This else clause
will now get executed not only if options->cert_file is false, but
also if its true and the call to tls_ctx_use_external_private_key()
succeeds! That would be wrong and is not what we want.

>
>  {
> -char *external_certificate = management_query_cert(management,
> -   
> options->management_certificate);
> -tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
> - external_certificate);
> -free(external_certificate);
> +char *external_cert = management_query_cert(
> +management, options->management_certificate);
> +
>
> +if (!external_cert)
> +{
> +msg(M_FATAL, "Failed to initialize 
> management-external-cert");
> +}
> +
> +if (0 != tls_ctx_use_external_private_key(new_ctx, 
> INLINE_FILE_TAG,
> +  external_cert))
> +{
> +msg(M_FATAL, "Failed to initialize management-external-key");
> +}
> +
> +free(external_cert);
>  }
>  }
>  #endif
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 8ef68ebd..4d434fa2 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1330,7 +1330,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx 
> *ctx,
>  return 0;
>
>  err:
> -crypto_msg(M_FATAL, "Cannot enable SSL external private key capability");
> +crypto_msg(M_WARN, "Cannot enable SSL external private key capability");
>  return 1;
>  }


Selva

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/2] Skip expired certificates in Windows certificate store

[Steffan Karger]

Hi,

On 02-04-18 16:58, Selva Nair wrote:
> On Mon, Apr 2, 2018 at 8:37 AM, Steffan Karger  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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Properly respond to SIGTERM received during DNS resolution.

[Steffan Karger]

Hi,

On 06-02-18 06:53, Selva Nair wrote:
> On Mon, Feb 5, 2018 at 7:52 PM, Jonathan K. Bullard  
> wrote:
>> Hi, I'd like to reopen this patch -- it seems to have gotten lost.
>>
>> The patch is so old the line numbers are wrong but the code doesn't
>> seem to have changed.
>>
>> I'm top-posting because this thread doesn't show up in the SourceForge
>> archive[1] (???) so I've extracted the relevant parts below. There was
>> some top-posting but nothing was really out-of-order, so I've tried to
>> make the thread intelligible and include everything relevant below.
>>
>> On Tue, Apr 12, 2016 at 11:42 AM, Fish  wrote:
>>> In `link_socket_init_phase2`, it used to be the case that any received
>>> signal will be overwritten by SIGUSR1 if the socket cannot be created
>>> (e.g. when DNS resolution fails), which consequently prevents OpenVPN from
>>> responding to SIGTERM and exiting. This patch adds an additional check of
>>> whether the received signal during DNS resolution is SIGTERM or not, and
>>> prematurely exits from `link_socket_init_phase2` when receiving a SIGTERM.
>>> ---
>>>  src/openvpn/socket.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
>>> index 9bcf4d4..5e8abe1 100644
>>> --- a/src/openvpn/socket.c
>>> +++ b/src/openvpn/socket.c
>>> @@ -1905,6 +1905,11 @@ link_socket_init_phase2 (struct link_socket *sock,
>>> }
>>> }
>>>
>>> +  if (sig_info->signal_received == SIGTERM)
>>> +   {
>>> + goto done;
>>> +   }
>>> +
>>>/* Socket still undefined, give a warning and abort connection */
>>>if (sock->sd == SOCKET_UNDEFINED)
>>> {
>>
>> On Tue, Apr 12, 2016 at 11:46 AM, Gert Doering  wrote:
>>> That is the "on DNS failure, the client loops forever and is not killable"
>>> problem, right?
>>>
>>> (I'm not sure I'm reading the description right, to understand the
>>> actual issue this is fixing - but if I'm reading it right, then this
>>> makes sense :-)  - what about SIGINT?)
>>
>>
>> On Tue, Apr 12, 2016 at 11:48 AM, Fish Wang  wrote:
>>>
>>> Right, it's for the "on DNS failure, the client loops forever and is not 
>>> killable" problem.
>>
>>
>> On Tue, Apr 12, 2016 at 12:25 PM, Jonathan K. Bullard
>>  wrote:
>>> Feature ACK; this should make DNS hangs much easier to deal with for
>>> GUIs such as Tunnelblick.
>>>
>>> The "hang forever" problem can be avoided by using --resolve-retry.
>>>
>>
>> On Tue, Apr 12, 2016 at 1:18 PM, Arne Schwabe  wrote:
>>> Am 12.04.16 um 17:42 schrieb Fish:
 In `link_socket_init_phase2`, it used to be the case that any received
 signal will be overwritten by SIGUSR1 if the socket cannot be created
 (e.g. when DNS resolution fails)
>>>
>>> That should not happen in the first place, doesn't register_signal keep
>>> care of not overwriting higher priority signals?
>>
>> On Tue, Apr 12, 2016 at 10:07 PM, Fish Wang  wrote:
>>>
>>> No, the patch won't fix that issue. I may send out another patch later to 
>>> fix that problem if I have free time this week.
>>
>> On Tue, Apr 12, 2016 at 10:11 PM, Fish Wang  wrote:
>>> Check out the code in socket.c [1]. This is where the received signal is 
>>> overwritten by SIGUSR1.
>>>
>>>   /* Socket still undefined, give a warning and abort connection */
>>>   if (sock->sd == SOCKET_UNDEFINED)
>>> {
>>>   msg (M_WARN, "Could not determine IPv4/IPv6 protocol");
>>>   sig_info->signal_received = SIGUSR1;
>>>   goto done;
>>> }
>>> [1] 
>>> https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/socket.c#L1909
>>
>> Note that theses lines are immediately below the patch. The last three
>> lines of the patch (which are not changed in the patch) are the same
>> as the first three lines shown above.
> 
> This jogs my memory.
> 
> We do have signal overwrite issues in more than a couple of places in
> the code and signal handling after getaddrinfo failure does need
> fixing. I think this is a good opportunity to discuss whether we
> continue tip-toeing around this or take a closer look at signal handling
> and make some bolder changes.
> 
> When this patch was being discussed 2 years ago I had looked into our
> signal handling code. A number of issues and some solutions were
> identified but never finished for one reason or other. My stab at
> improving signal handling is in this feature branch:
> https://github.com/selvanair/openvpn/tree/signals-v5
> 
> I'd like to resurrect it but it would be useful to know whether there
> is any interest in a revamping the signals code.
> 
> For those who may not want look through code based on a 2 years
> old branch, with a longish commit _not_ yet split into bite sized pieces,
> here is a gist:
> 
> 1. In many places in the code, signal_received is directly 

Re: [Openvpn-devel] [PATCH 1/2] Skip expired certificates in Windows certificate store

[Steffan Karger]

Hi,

One comment based on stare-at-code only:

On 12-03-18 02:17, selva.n...@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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Improve management-external-key/cert error handling

[Steffan Karger]

Hi,

On 9 March 2018 at 04:38, Selva Nair  wrote:
> I wanted to give this a quick test, but it doesn't apply.
>
> It seems you have patch 116 (Antonio's "inline-tag changed to bool"
> patch) in your local repo.

Oops, you're right - this was based on top of my local working branch,
which indeed includes Antonio's inline-tag cleanup patch. Just sent a
version based on current public master.

> By the way, the M_FATAL after management_query_cert() looks like a
> regression. One problem with these FATAL exits is that it makes it
> hard for the GUI to gracefully handle when user presses cancel on some
> dialogs. Anyway that's beyond this patch... So just saying..

Not really a regression, right?  Previously, management_query_cert()
would return NULL, which would cause
tls_ctx_use_external_private_key() to ASSERT out, because
tls_ctx_load_cert_file_and_copy() would not be able to return a valid
"cert" pointer.

But you're definitely right that the error handling over the
management interface could benefit greatly from some rework. (Beyond
this patch, indeed.)

-Steffan

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v3] Improve management-external-key/cert error handling

[Steffan Karger]

Check the return values of management_query_cert() and
tls_ctx_use_external_private_key(), and error out with a more descriptive
error message.  To do so, we make the openssl-backed implementation of
tls_ctx_use_external_private_key() not throw fatal error anymore.

(And fix line wrapping while touching this code.)

Signed-off-by: Steffan Karger 
---
v2: error out with M_FATAL as suggested by Selva.
v3: rebase on master (without extra patches)

 src/openvpn/ssl.c | 28 
 src/openvpn/ssl_openssl.c |  2 +-
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 669f941b..b06820ba 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -660,18 +660,30 @@ init_ssl(const struct options *options, struct 
tls_root_ctx *new_ctx)
 else if ((options->management_flags & MF_EXTERNAL_KEY)
  && (options->cert_file || options->management_flags & 
MF_EXTERNAL_CERT))
 {
-if (options->cert_file)
+if (options->cert_file
+&& 0 != tls_ctx_use_external_private_key(new_ctx,
+ options->cert_file,
+ 
options->cert_file_inline))
 {
-tls_ctx_use_external_private_key(new_ctx, options->cert_file,
- options->cert_file_inline);
+msg(M_FATAL, "Failed to initialize management-external-key");
 }
 else
 {
-char *external_certificate = management_query_cert(management,
-   
options->management_certificate);
-tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
- external_certificate);
-free(external_certificate);
+char *external_cert = management_query_cert(
+management, options->management_certificate);
+
+if (!external_cert)
+{
+msg(M_FATAL, "Failed to initialize management-external-cert");
+}
+
+if (0 != tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
+  external_cert))
+{
+msg(M_FATAL, "Failed to initialize management-external-key");
+}
+
+free(external_cert);
 }
 }
 #endif
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 8ef68ebd..4d434fa2 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1330,7 +1330,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
 return 0;
 
 err:
-crypto_msg(M_FATAL, "Cannot enable SSL external private key capability");
+crypto_msg(M_WARN, "Cannot enable SSL external private key capability");
 return 1;
 }
 
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Depreciate IPv4-related options.

[Marvin Adeff]

Antonio,
I certainly don’t disagree with you. 

However I think I’ve taken up enough bandwidth over this topic on 
Openvpn-devel. Thank you all. 

Marvin

> On Apr 1, 2018, at 7:20 PM, Antonio Quartulli  wrote:
> 
>> On 02/04/18 10:12, Marvin Adeff wrote:
>> Even on the internet I can tell country, ISP etc. Very useful for security 
>> ACLs etc. Unless I’m completely mistaken, I don’t believe this is easily 
>> done in ipv6. 
> 
> mostly because at this very moment Tunnel Brokers are widely used and
> they act as a "proxy", effectively covering the real location of the
> client host.
> 
> Many websites just show you (client) as connecting from the country
> where your Tunnel Broker is located.
> 
> When using native IPv6 this problem does not exists anymore.
> 
> Therefore, the proper way to get over this "limitation" (even though I
> don't think is a real problem, but this is of course my perspective) is
> to speed up the transition and move everybody over native IPv6 (which is
> something we can't achieve if we continue to be "afraid" of using IPv6
> in our everyday life).
> 
> Cheers,
> 
> -- 
> Antonio Quartulli
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel