[Openvpn-devel] combined ndis5 + ndis6 installer ?

2016-11-29 Thread Илья Шипицин
Hello,

as we finished x86 + x64 installer, we can do something else now.
@mattock, which installer are you going to build ?

it used to be (ndis5, ndis6) x (x86, x64) matrix, what will be future
matrix ?

(and, yes, I'm going to build multi-language installer, probably right
after 2.4 release)

Cheers,
Ilya Shipitsin
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 2.4 and 2.3] When parsing "--setenv opt xx .." make sure a third parameter is present

2016-11-29 Thread Selva Nair
When no parameters are present, set it to "setenv opt" to trigger a
descriptive error message. And, thus get rid of the pesky NULL pointer
dereferencing.

Resolves Trac 779

Signed-off-by: Selva Nair 
---
 src/openvpn/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c540e05..d4ee340 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -4425,6 +4425,8 @@ add_option (struct options *options,
*/
   if (streq (p[0], "setenv") && p[1] && streq (p[1], "opt") && 
!(permission_mask & OPT_P_PULL_MODE))
 {
+  if (!p[2])
+p[2] = "setenv opt"; /* will trigger an error that includes setenv opt 
*/
   p += 2;
   msglevel_fc = M_WARN;
 }
-- 
2.1.4


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2.3] Map restart signals from event loop to SIGTERM during exit-notification wait

2016-11-29 Thread Selva Nair
Commit 63b3e000c9.. fixed SIGTERM getting lost during exit notification
by ignoring any restart signals triggered during this interval. However,
as reported in Trac 777, this could result in repeated triggering of
restart signals when the event loop cannot continue without restart due
to IO errors or timeout.

Avoid by converting soft SIGUSR1 and SIGHUP signals received during
exit-notify wait period to SIGTERM.

cherry-picked from commit f25a0217e35f53c3110ebb226e1d1f3528152cb5
with (c->sig->source == SIG_SOURCE_HARD) changed to c->sig->hard

Signed-off-by: Selva Nair 
---
 src/openvpn/sig.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index 87f2cdb..b17e166 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -362,7 +362,8 @@ process_sigterm (struct context *c)
 
 /**
  * If a restart signal is received during exit-notification, reset the
- * signal and return true.
+ * signal and return true. If its a soft restart signal from the event loop
+ * which implies the loop cannot continue, remap to SIGTERM to exit promptly.
  */
 static bool
 ignore_restart_signals (struct context *c)
@@ -372,10 +373,20 @@ ignore_restart_signals (struct context *c)
   if ( (c->sig->signal_received == SIGUSR1 || c->sig->signal_received == 
SIGHUP) &&
 event_timeout_defined(>c2.explicit_exit_notification_interval) )
 {
-   msg (M_INFO, "Ignoring %s received during exit notification",
-signal_name(c->sig->signal_received, true));
-   signal_reset (c->sig);
-   ret = true;
+   if (c->sig->hard)
+ {
+msg (M_INFO, "Ignoring %s received during exit notification",
+ signal_name(c->sig->signal_received, true));
+signal_reset (c->sig);
+ret = true;
+ }
+   else
+ {
+msg (M_INFO, "Converting soft %s received during exit notification 
to SIGTERM",
+ signal_name(c->sig->signal_received, true));
+register_signal(c, SIGTERM, "exit-with-notification");
+ret = false;
+ }
 }
 #endif
   return ret;
-- 
2.1.4


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/1] Use systemd service manager notification

2016-11-29 Thread David Sommerseth
On 29/11/16 16:27, Christian Hesse wrote:
> From: Christian Hesse 
> 
> Notify systemd service manager when our initialization sequence
> completed. This helps ordering services as dependencies can rely on vpn
> being available.

Funny detail is that I have a somewhat similar patch in a local git
tree, awaiting proper testing ... I postponed it as this is not
something we will pull into v2.4.  We're going to release 2.4_rc1 this
week, and that is too late for more intrusive changes (even though the
changeset itself is small, the code changes makes OpenVPN behave
somewhat different when managed by systemd).

Just a question, as it is good to see more people looking into these
code paths ... I was considering to extend my approach to update STATUS=
a bit more frequently.  On the client side, I thought it would be good
if the status line had "Resolving %s", "Connecting to %s", "Successful
connection to %s" or "Failed to connect to %s".  On the server side I
was pondering on a "Successfully started, %i clients connected".  What
do you think about that?  Does the sd_notify() API support more frequent
updates?

Also when using Type=notify ... does systemd expect the OpenVPN process
to fork into the background or run in the foreground as now?


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc








> ---
>  distro/systemd/openvpn-client@.service | 1 +
>  distro/systemd/openvpn-server@.service | 1 +
>  src/openvpn/init.c | 6 ++
>  src/openvpn/init.h | 4 
>  4 files changed, 12 insertions(+)
> 
> diff --git a/distro/systemd/openvpn-client@.service 
> b/distro/systemd/openvpn-client@.service
> index 18b84dd..f64a239 100644
> --- a/distro/systemd/openvpn-client@.service
> +++ b/distro/systemd/openvpn-client@.service
> @@ -7,6 +7,7 @@ 
> Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
>  Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
>  
>  [Service]
> +Type=notify
>  PrivateTmp=true
>  RuntimeDirectory=openvpn-client
>  RuntimeDirectoryMode=0710
> diff --git a/distro/systemd/openvpn-server@.service 
> b/distro/systemd/openvpn-server@.service
> index a2b7b52..890e6a9 100644
> --- a/distro/systemd/openvpn-server@.service
> +++ b/distro/systemd/openvpn-server@.service
> @@ -7,6 +7,7 @@ 
> Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
>  Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
>  
>  [Service]
> +Type=notify
>  PrivateTmp=true
>  RuntimeDirectory=openvpn-server
>  RuntimeDirectoryMode=0710
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 2ccbab2..551e579 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1251,10 +1251,16 @@ initialization_sequence_completed (struct context *c, 
> const unsigned int flags)
>show_adapters (M_INFO|M_NOPREFIX);
>msg (M_INFO, "%s With Errors ( see 
> http://openvpn.net/faq.html#dhcpclientserv )", message);
>  #else
> +#ifdef ENABLE_SYSTEMD
> +  sd_notifyf(0, "STATUS=Failed to start up: %s With Errors\nERRNO=1", 
> message);
> +#endif /* HAVE_SYSTEMD_SD_DAEMON_H */
>msg (M_INFO, "%s With Errors", message);
>  #endif
>  }
>else
> +#ifdef ENABLE_SYSTEMD
> +sd_notifyf(0, "READY=1\nSTATUS=%s\nMAINPID=%lu", message, (unsigned 
> long) getpid());
> +#endif
>  msg (M_INFO, "%s", message);
>  
>/* Flag that we initialized */
> diff --git a/src/openvpn/init.h b/src/openvpn/init.h
> index 524bc64..0518b06 100644
> --- a/src/openvpn/init.h
> +++ b/src/openvpn/init.h
> @@ -27,6 +27,10 @@
>  
>  #include "openvpn.h"
>  
> +#ifdef ENABLE_SYSTEMD
> +#include 
> +#endif
> +
>  /*
>   * Baseline maximum number of events
>   * to wait for.
> 




signature.asc
Description: OpenPGP digital signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Antonio Quartulli
On Tue, Nov 29, 2016 at 05:43:33PM +0100, Steffan Karger wrote:

[CUT..]

> >  
> > +void
> > +tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
> > +  const char *crl_file_inline, bool reload)
> 
> This is only used within ssl.c, so should be static and have a function
> declaration, preferably doxygen-documented, at the beginning of ssl.c.

You're absolutely right about this function supposed to be static. I missed
that.

Do I really need to split declaration and definition? Or can I just
doxy-document the definition in the same place where it is now?

> 
> > +{
> > +  /* if something goes wrong with stat(), we'll store 0 as mtime */
> > +  platform_stat_t crl_stat = {0};
> > +
> > +  /*
> > +   * an inline CRL can't change at runtime, therefore there is no need to
> > +   * reload it. It will be reloaded upon config change + SIGHUP.
> > +   */
> > +  if (crl_file_inline && reload)
> > +return;
> > +
> > +  if (!crl_file_inline)
> > +platform_stat(crl_file, _stat);
> > +
> > +  /*
> > +   * If this is not a reload, update the CRL right away.
> > +   * Otherwise, update only if the CRL file was modified after the last 
> > reload.
> > +   * Note: Windows does not support tv_nsec.
> > +   */
> > +  if (reload &&
> > +  (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime))
> > +return;
> > +
> > +  ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
> > +  backend_tls_ctx_reload_crl(ssl_ctx, crl_file, crl_file_inline);
> > +}
> 
> This can be done without the 'reload' argument too, something like:
> 
>   /* if something goes wrong with stat(), we'll store 0 as mtime */
>   platform_stat_t crl_stat = {0};
> 
>   if (crl_file_inline)
> {
>   /*
>* an inline CRL can't change at runtime, therefore there is no
> need to
>* reload it. It will be reloaded upon config change + SIGHUP.
>*/
>   if (ssl_ctx->crl_last_mtime.tv_sec)
>   return;
>   else
> crl_stat.st_mtime = now; /* Set dummy mtime */
> }
>   else
> {
>   platform_stat (crl_file, _stat);
> }
> 
>   /*
>* If this is not a reload, update the CRL right away.
>* Otherwise, update only if the CRL file was modified after the last
> reload.
>* Note: Windows does not support tv_nsec.
>*/
>   if (ssl_ctx->crl_last_mtime.tv_sec <= crl_stat.st_mtime)
> {
>   ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
>   backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline);
> }
> 
> I slightly prefer this over adding the extra argument, but can live with
> your approach just fine too.  Pick what you like best :)

I guess the assumption here is that crl_last_mtime.tv_sec is initialized with 0.
This would make your code work. And yeah, I also prefer to avoid the reload
argument ;)


Will make these changes and resend the patch.

Thanks for the comments!

Cheers,

-- 
Antonio Quartulli

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Force 'def1' method when --redirect-gateway is done through service

2016-11-29 Thread Selva Nair
The service deletes all added routes when the client process (openvpn)
exits, causing the re-instated default route to disappear.
Fix by rewriting "--redirect-gateway" to "--redirect-gateway def1" when
routes are set using interactive service.

Only the behaviour on Windows with intereactive service is affected.

Resolves Trac 778

Signed-off-by: Selva Nair 
---
 src/openvpn/options.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 63dcc24..c540e05 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2526,6 +2526,22 @@ options_postprocess_mutate_ce (struct options *o, struct 
connection_entry *ce)
 
 }
 
+#ifdef _WIN32
+/* If iservice is in use, we need def1 method for redirect-gateway */
+static void
+remap_redirect_gateway_flags (struct options *opt)
+{
+  if (opt->routes
+  && opt->route_method == ROUTE_METHOD_SERVICE
+  && opt->routes->flags & RG_REROUTE_GW
+  && !(opt->routes->flags & RG_DEF1))
+{
+  msg (M_INFO, "Flag 'def1' added to --redirect-gateway (iservice is in 
use)");
+  opt->routes->flags |= RG_DEF1;
+}
+}
+#endif
+
 static void
 options_postprocess_mutate_invariant (struct options *options)
 {
@@ -2555,6 +2571,8 @@ options_postprocess_mutate_invariant (struct options 
*options)
   options->tuntap_options.ip_win32_type = IPW32_SET_MANUAL;
   options->ifconfig_noexec = false;
 }
+
+  remap_redirect_gateway_flags (options);
 #endif
 
 #if P2MP_SERVER
@@ -5705,6 +5723,10 @@ add_option (struct options *options,
  goto err;
}
}
+#ifdef _WIN32
+  /* we need this here to handle pushed --redirect-gateway */
+  remap_redirect_gateway_flags (options);
+#endif
   options->routes->flags |= RG_ENABLE;
 }
   else if (streq (p[0], "remote-random-hostname") && !p[1])
-- 
2.1.4


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH applied] Re: Map restart signals from event loop to SIGTERM during exit-notification wait

2016-11-29 Thread Gert Doering
Hi,

On Tue, Nov 29, 2016 at 04:01:34PM -0500, Selva Nair wrote:
> Just to be sure, I think the second ctrl-C or SIGTERM during exit-notify
> wait should cause a prompt exit without waiting for exit-notify to
> complete. That was the original behaviour and is supposed to be unchanged...

It does :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH applied] Re: Map restart signals from event loop to SIGTERM during exit-notification wait

2016-11-29 Thread Selva Nair
On Tue, Nov 29, 2016 at 3:04 AM, Gert Doering  wrote:

> .. which fixes the issue for good.
>
> "Real signals" are still ignored (kill -USR1 from another window):
>
> Tue Nov 29 08:58:19 2016 event_wait : Interrupted system call (code=4)
> Tue Nov 29 08:58:19 2016 Ignoring SIGUSR1 received during exit notification
>
> (or pressing ctrl-c a second time while it's waiting for the exit
> notification
> to proceed)
>

Just to be sure, I think the second ctrl-C or SIGTERM during exit-notify
wait should cause a prompt exit without waiting for exit-notify to
complete. That was the original behaviour and is supposed to be unchanged...

Selva
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger

On 29-11-16 17:43, Steffan Karger wrote:
> Will test more thoroughly tonight (hopefully on Windows too), but have a
> lot of faith that those will succeed.

Just did this, on linux and windows, and code works as expected.  So
once you've taken care of the (minor) remarks in my previous mails, I
think this patch is good to go!

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger
Hi,

One more thing.

On 29-11-16 07:38, Antonio Quartulli wrote:
> +  if (!crl_file_inline)
> +platform_stat(crl_file, _stat);

If platform_stat() fails, we now silently ignore the error and continue
using the old CRL.  I think using the old CRL is fine, but we should
print a warning to tell the user so:

  if (!crl_file_inline && 0 != platform_stat(crl_file, _stat))
msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Christian Hesse
Christian Hesse  on Tue, 2016/11/29 20:16:
> Oops, missed that in my logs (and did not find the code)... You are right,
> cache is cleared.
> 
> Either of both is just fine and it works as-is. So ignore my patch.

Oops again... Looks like I answered a wrong mail. Please ignore... (The
mail, not any patch. ;)
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpBSCfFHb2Lq.pgp
Description: OpenPGP digital signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Christian Hesse
Steffan Karger  on Tue, 2016/11/29 17:43:
> Hi,
> 
> Thanks for following up.  I did some stare-at-code and trivial tests.
> Will test more thoroughly tonight (hopefully on Windows too), but have a
> lot of faith that those will succeed.  I have some comments from staring
> at the code though, see below.

Oops, missed that in my logs (and did not find the code)... You are right,
cache is cleared.

Either of both is just fine and it works as-is. So ignore my patch.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgp7MmI2_Fygp.pgp
Description: OpenPGP digital signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger
Hi,

Thanks for following up.  I did some stare-at-code and trivial tests.
Will test more thoroughly tonight (hopefully on Windows too), but have a
lot of faith that those will succeed.  I have some comments from staring
at the code though, see below.

On 29-11-16 07:38, Antonio Quartulli wrote:
> In order to prevent annoying delays upon client connection,
> reload the CRL file only if it was modified since the last
> reload operation.
> If not, keep on using the already stored CRL.
> 
> This change will boost client connection time in instances
> where the CRL file is quite large (dropping from several
> seconds to few milliseconds).
> 
> Cc: Steffan Karger 
> Signed-off-by: Antonio Quartulli 
> ---
> 
> Tested on linux by using my VM.
> No test was performed on Windows* (compiled-only).
> 
> Note: the check "!(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))" may not
> always work as expected. The user may forge a wrong, but still compliant,
> configuration that would fool this test. This issue exists even before 
> applying
> this patch.
> 
> Cheers,
> 
>  src/openvpn/ssl.c | 40 +---
>  src/openvpn/ssl_backend.h |  2 +-
>  src/openvpn/ssl_mbedtls.c |  2 +-
>  src/openvpn/ssl_mbedtls.h |  1 +
>  src/openvpn/ssl_openssl.c |  2 +-
>  src/openvpn/ssl_openssl.h |  1 +
>  6 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 7347a78..235f7df 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -612,7 +612,8 @@ init_ssl (const struct options *options, struct 
> tls_root_ctx *new_ctx)
>/* Read CRL */
>if (options->crl_file && !(options->ssl_flags & SSLF_CRL_VERIFY_DIR))
>  {
> -  tls_ctx_reload_crl(new_ctx, options->crl_file, 
> options->crl_file_inline);
> +  tls_ctx_reload_crl(new_ctx, options->crl_file, 
> options->crl_file_inline,
> +  false);
>  }
>  
>/* Once keys and cert are loaded, load ECDH parameters */
> @@ -2450,6 +2451,36 @@ auth_deferred_expire_window (const struct tls_options 
> *o)
>return ret;
>  }
>  
> +void
> +tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
> +const char *crl_file_inline, bool reload)

This is only used within ssl.c, so should be static and have a function
declaration, preferably doxygen-documented, at the beginning of ssl.c.

> +{
> +  /* if something goes wrong with stat(), we'll store 0 as mtime */
> +  platform_stat_t crl_stat = {0};
> +
> +  /*
> +   * an inline CRL can't change at runtime, therefore there is no need to
> +   * reload it. It will be reloaded upon config change + SIGHUP.
> +   */
> +  if (crl_file_inline && reload)
> +return;
> +
> +  if (!crl_file_inline)
> +platform_stat(crl_file, _stat);
> +
> +  /*
> +   * If this is not a reload, update the CRL right away.
> +   * Otherwise, update only if the CRL file was modified after the last 
> reload.
> +   * Note: Windows does not support tv_nsec.
> +   */
> +  if (reload &&
> +  (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime))
> +return;
> +
> +  ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
> +  backend_tls_ctx_reload_crl(ssl_ctx, crl_file, crl_file_inline);
> +}

This can be done without the 'reload' argument too, something like:

  /* if something goes wrong with stat(), we'll store 0 as mtime */
  platform_stat_t crl_stat = {0};

  if (crl_file_inline)
{
  /*
   * an inline CRL can't change at runtime, therefore there is no
need to
   * reload it. It will be reloaded upon config change + SIGHUP.
   */
  if (ssl_ctx->crl_last_mtime.tv_sec)
return;
  else
crl_stat.st_mtime = now; /* Set dummy mtime */
}
  else
{
  platform_stat (crl_file, _stat);
}

  /*
   * If this is not a reload, update the CRL right away.
   * Otherwise, update only if the CRL file was modified after the last
reload.
   * Note: Windows does not support tv_nsec.
   */
  if (ssl_ctx->crl_last_mtime.tv_sec <= crl_stat.st_mtime)
{
  ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
  backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline);
}

I slightly prefer this over adding the extra argument, but can live with
your approach just fine too.  Pick what you like best :)

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Introduce and use secure_memzero() to erase secrets

2016-11-29 Thread Selva Nair
Hi,

On Mon, Nov 28, 2016 at 5:14 PM, Steffan Karger  wrote:

> As described in trac #751, and shortly after reported by Zhaomo Yang, of
> the University of California, San Diego, we use memset() (often through
> the CLEAR() macro) to erase secrets after use.  In some cases however, the
> compiler might optimize these calls away.
>
> This patch replaces these memset() calls on secrets by calls to a new
> secure_memzero() function, that will not be optimized away.
>
> Since we use CLEAR() a LOT of times, I'm not changing that to use
> secure_memzero() to prevent performance impact.  I did annotate the macro
> to point people at secure_memzero().
>
> This patch also replaces some CLEAR() or memset() calls with a zero-
> initialization using "= { 0 }" if that has the same effect, because that
> better captures the intend of that code.
>
> Signed-off-by: Steffan Karger 
> ---
> v2: replace more memset()/CLEAR() calls with secure_memzero()
>
>  src/openvpn/basic.h  |  2 +-
>  src/openvpn/buffer.c |  8 
>  src/openvpn/buffer.h | 43 ++
> ++
>  src/openvpn/console_builtin.c|  2 +-
>  src/openvpn/crypto.c | 17 ++--
>  src/openvpn/manage.c |  2 +-
>  src/openvpn/misc.c   |  4 ++--
>  src/openvpn/options.c|  6 +++---
>  src/openvpn/ssl.c| 32 +++---
>  src/openvpn/ssl_verify.c |  4 ++--
>  src/openvpn/ssl_verify_mbedtls.c |  4 +---
>  11 files changed, 80 insertions(+), 44 deletions(-)


Took a second look at v2: sizes of data structs/buffers correctly passed to
secure_memset and no new warnings with gcc -Wall :)

ACK.

Selva
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 1/1] Use systemd service manager notification

2016-11-29 Thread Christian Hesse
From: Christian Hesse 

Notify systemd service manager when our initialization sequence
completed. This helps ordering services as dependencies can rely on vpn
being available.

Signed-off-by: Christian Hesse 
---
 distro/systemd/openvpn-client@.service | 1 +
 distro/systemd/openvpn-server@.service | 1 +
 src/openvpn/init.c | 6 ++
 src/openvpn/init.h | 4 
 4 files changed, 12 insertions(+)

diff --git a/distro/systemd/openvpn-client@.service 
b/distro/systemd/openvpn-client@.service
index 18b84dd..f64a239 100644
--- a/distro/systemd/openvpn-client@.service
+++ b/distro/systemd/openvpn-client@.service
@@ -7,6 +7,7 @@ 
Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
 Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
 
 [Service]
+Type=notify
 PrivateTmp=true
 RuntimeDirectory=openvpn-client
 RuntimeDirectoryMode=0710
diff --git a/distro/systemd/openvpn-server@.service 
b/distro/systemd/openvpn-server@.service
index a2b7b52..890e6a9 100644
--- a/distro/systemd/openvpn-server@.service
+++ b/distro/systemd/openvpn-server@.service
@@ -7,6 +7,7 @@ 
Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
 Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
 
 [Service]
+Type=notify
 PrivateTmp=true
 RuntimeDirectory=openvpn-server
 RuntimeDirectoryMode=0710
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 2ccbab2..551e579 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1251,10 +1251,16 @@ initialization_sequence_completed (struct context *c, 
const unsigned int flags)
   show_adapters (M_INFO|M_NOPREFIX);
   msg (M_INFO, "%s With Errors ( see 
http://openvpn.net/faq.html#dhcpclientserv )", message);
 #else
+#ifdef ENABLE_SYSTEMD
+  sd_notifyf(0, "STATUS=Failed to start up: %s With Errors\nERRNO=1", 
message);
+#endif /* HAVE_SYSTEMD_SD_DAEMON_H */
   msg (M_INFO, "%s With Errors", message);
 #endif
 }
   else
+#ifdef ENABLE_SYSTEMD
+sd_notifyf(0, "READY=1\nSTATUS=%s\nMAINPID=%lu", message, (unsigned long) 
getpid());
+#endif
 msg (M_INFO, "%s", message);
 
   /* Flag that we initialized */
diff --git a/src/openvpn/init.h b/src/openvpn/init.h
index 524bc64..0518b06 100644
--- a/src/openvpn/init.h
+++ b/src/openvpn/init.h
@@ -27,6 +27,10 @@
 
 #include "openvpn.h"
 
+#ifdef ENABLE_SYSTEMD
+#include 
+#endif
+
 /*
  * Baseline maximum number of events
  * to wait for.
-- 
2.10.2


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/1] Clean up plugin path handling

2016-11-29 Thread Christian Hesse
Christian Hesse  on Tue, 2016/11/29 12:07:
> From: Christian Hesse 
> 
> Drop --with-plugindir, instead use an environment variable PLUGINDIR
> to specify the plugin directory.
> 
> This always defines PLUGIN_LIBDIR and enables plugin search path.
> 
> Signed-off-by: Christian Hesse 
> ---
>  configure.ac| 14 ++
>  src/openvpn/Makefile.am |  3 ++-
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index f4073d0..5fe652e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -301,13 +301,12 @@ AC_ARG_WITH(
>   [with_crypto_library="openssl"]
>  )
>  
> -AC_ARG_WITH(
> - [plugindir],
> - [AS_HELP_STRING([--with-plugindir], [plugin directory
> @<:@default=LIBDIR/openvpn@:>@])],
> - ,
> - [with_plugindir="\$(libdir)/openvpn/plugins"]
> -)
> -
> +AC_ARG_VAR([PLUGINDIR], [Path of plug-in directory
> @<:@default=LIBDIR/openvpn/plugins@:>@]) +if test -n "${PLUGINDIR}"; then
> + plugindir="${PLUGINDIR}"
> +else
> + plugindir="\${libdir}/openvpn/plugins"
> +fi
>  
>  AC_DEFINE_UNQUOTED([TARGET_ALIAS], ["${host}"], [A string representing our
> host]) case "$host" in
> @@ -1245,7 +1244,6 @@ AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test
> "${enable_plugin_auth_pam}" = "ye AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT],
> [test "${enable_plugin_down_root}" = "yes"])
> AM_CONDITIONAL([ENABLE_CRYPTO], [test "${enable_crypto}" = "yes"]) 
> -plugindir="${with_plugindir}"
>  sampledir="\$(docdir)/sample"
>  AC_SUBST([plugindir])
>  AC_SUBST([sampledir])
> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
> index 4c18449..188834a 100644
> --- a/src/openvpn/Makefile.am
> +++ b/src/openvpn/Makefile.am
> @@ -27,7 +27,8 @@ AM_CFLAGS = \
>   $(OPTIONAL_CRYPTO_CFLAGS) \
>   $(OPTIONAL_LZO_CFLAGS) \
>   $(OPTIONAL_LZ4_CFLAGS) \
> - $(OPTIONAL_PKCS11_HELPER_CFLAGS)
> + $(OPTIONAL_PKCS11_HELPER_CFLAGS) \
> + -DPLUGIN_LIBDIR=\"${plugindir}\"
>  if WIN32
>  # we want unicode entry point but not the macro
>  AM_CFLAGS += -municode -UUNICODE

The alternative would look something like this:

--- a/configure.ac
+++ b/configure.ac
@@ -301,13 +301,13 @@ AC_ARG_WITH(
[with_crypto_library="openssl"]
 )
 
-AC_ARG_WITH(
-   [plugindir],
-   [AS_HELP_STRING([--with-plugindir], [plugin directory 
@<:@default=LIBDIR/openvpn@:>@])],
-   ,
-   [with_plugindir="\$(libdir)/openvpn/plugins"]
-)
-
+AC_ARG_VAR([PLUGINDIR], [Path of plug-in directory 
@<:@default=LIBDIR/openvpn/plugins@:>@])
+if test -n "${PLUGINDIR}"; then
+   plugindir="${PLUGINDIR}"
+   AC_DEFINE_UNQUOTED([PLUGIN_LIBDIR], ["${PLUGINDIR}"], [Path of plug-in 
search directory])
+else
+   plugindir="\${libdir}/openvpn/plugins"
+fi
 
 AC_DEFINE_UNQUOTED([TARGET_ALIAS], ["${host}"], [A string representing our 
host])
 case "$host" in
@@ -1245,7 +1245,6 @@ AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test 
"${enable_plugin_auth_pam}" = "ye
 AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT], [test "${enable_plugin_down_root}" = 
"yes"])
 AM_CONDITIONAL([ENABLE_CRYPTO], [test "${enable_crypto}" = "yes"])
 
-plugindir="${with_plugindir}"
 sampledir="\$(docdir)/sample"
 AC_SUBST([plugindir])
 AC_SUBST([sampledir])

However you *have* to give PLUGINDIR this way to enable the search path. I
would like to avoid that. And I did not find a way to move
AC_DEFINE_UNQUOTED() below the condition due to the nested variables in
$libdir.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgp65zxVT78KU.pgp
Description: OpenPGP digital signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Introduce and use secure_memzero() to erase secrets

2016-11-29 Thread Steffan Karger
On 29-11-16 11:28, Arne Schwabe wrote:
> 
> 
> Am 29.11.16 um 11:18 schrieb Gert Doering:
>> Hi,
>>
>> On Tue, Nov 29, 2016 at 11:07:12AM +0100, Arne Schwabe wrote:
>>> I think we should prefer memset_s if available since it is a C11 standard.
>> Since we're currently compiling against C99, we can't rely on it being
>> there (in my OS Zoo, only MacOS seems to have it, at least "have a manpage
>> for it" - not even FreeBSD 11 or "gentoo current") - but then, configure 
>> could check that for us...
> I wouldn't add a configure test for that. According to
> http://en.cppreference.com/w/c/string/byte/memset:
> 
> As all bounds-checked functions, |memset_s| is only guaranteed to be
> available if __STDC_LIB_EXT1__ is defined by the implementation and if
> the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before
> including |string.h|.
> 
> so preprocessor magic can check that.

This is indeed how it should be done (and almost how the new version of
Yang et al does it).  However, memset_s() is part of the *optional*
annex K of C11, and there seems to be no agreement on whether it is a
good idea to implemented at all[0].

Given that Windows has SecureZeroMemory() and the memory barrier
approach works fine on the unixes, I don't see much gain in adding
support for memset_s().  Even so, I won't object to a patch adding it,
and will send a v3 if there are more people who want it.

-Steffan

[0] http://comments.gmane.org/gmane.comp.lib.glibc.alpha/44430


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 1/1] Clean up plugin path handling

2016-11-29 Thread Christian Hesse
From: Christian Hesse 

Drop --with-plugindir, instead use an environment variable PLUGINDIR
to specify the plugin directory.

This always defines PLUGIN_LIBDIR and enables plugin search path.

Signed-off-by: Christian Hesse 
---
 configure.ac| 14 ++
 src/openvpn/Makefile.am |  3 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index f4073d0..5fe652e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -301,13 +301,12 @@ AC_ARG_WITH(
[with_crypto_library="openssl"]
 )
 
-AC_ARG_WITH(
-   [plugindir],
-   [AS_HELP_STRING([--with-plugindir], [plugin directory 
@<:@default=LIBDIR/openvpn@:>@])],
-   ,
-   [with_plugindir="\$(libdir)/openvpn/plugins"]
-)
-
+AC_ARG_VAR([PLUGINDIR], [Path of plug-in directory 
@<:@default=LIBDIR/openvpn/plugins@:>@])
+if test -n "${PLUGINDIR}"; then
+   plugindir="${PLUGINDIR}"
+else
+   plugindir="\${libdir}/openvpn/plugins"
+fi
 
 AC_DEFINE_UNQUOTED([TARGET_ALIAS], ["${host}"], [A string representing our 
host])
 case "$host" in
@@ -1245,7 +1244,6 @@ AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test 
"${enable_plugin_auth_pam}" = "ye
 AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT], [test "${enable_plugin_down_root}" = 
"yes"])
 AM_CONDITIONAL([ENABLE_CRYPTO], [test "${enable_crypto}" = "yes"])
 
-plugindir="${with_plugindir}"
 sampledir="\$(docdir)/sample"
 AC_SUBST([plugindir])
 AC_SUBST([sampledir])
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index 4c18449..188834a 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -27,7 +27,8 @@ AM_CFLAGS = \
$(OPTIONAL_CRYPTO_CFLAGS) \
$(OPTIONAL_LZO_CFLAGS) \
$(OPTIONAL_LZ4_CFLAGS) \
-   $(OPTIONAL_PKCS11_HELPER_CFLAGS)
+   $(OPTIONAL_PKCS11_HELPER_CFLAGS) \
+   -DPLUGIN_LIBDIR=\"${plugindir}\"
 if WIN32
 # we want unicode entry point but not the macro
 AM_CFLAGS += -municode -UUNICODE
-- 
2.10.2


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] allow to enable plugin lookup with configure argument

2016-11-29 Thread Christian Hesse
David Sommerseth  on Tue, 2016/11/29 00:47:
> On 28/11/16 17:16, Christian Hesse wrote:
> > From: Christian Hesse 
> > 
> > For plugin lookup (give relative path to plugin directory in
> > configuration) we had to configure with something like this:
> > 
> > CFLAGS="$CFLAGS
> > -DPLUGIN_LIBDIR=\\\"/usr/lib/openvpn/plugins\\\"" ./configure
> > 
> > This allows to pass --enable-plugin-lookup to configure to achieve the
> > same. As a bonus we can be sure that install path and lookup path in
> > openvpn binary are the same.  
> 
> 
> Thank you for your patch.  Unfortunately I'm not convinced this is the
> proper way to do it.
> 
> First of all, to achieve what I believe is your goal, you need to flip
> things around:
> 
>   ./configure CFLAGS="$CFLAGS
> -DPLUGIN_LIBDIR=\\\"/usr/lib/openvpn/plugins\\\""
> 
> This will ensure that whenever 'make' decides to re-run ./configure
> automatically it will keep all variables provided to the command line.

Ah, right...
I used this to build a binary package, so configure is run only once and it
works for this case. :D

> Secondly, I believe the proper way to configure PLUGIN_LIBDIR without
> going via CFLAGS is to use a similar approach to what is used by
> IFCONFIG, ROUTE, IPROUTE and NETSTAT.  They are configured via AC_ARG_VAR.
> 
> I'd recommend just adding these two lines to configure.ac instead:
> 
> AC_ARG_VAR([PLUGINDIR], [Path of default plug-in search directory])
> AC_DEFINE_UNQUOTED([PLUGIN_LIBDIR], ["$PLUGINDIR"])
> 
> (these lines are not tested, but that should give some pointer towards a
> better direction.
> 
> With these lines in place, it is expected that you can do:
> 
>   ./configure PLUGINDIR=/usr/lib/openvpn/plugins
> 
> Which should result in defining PLUGIN_LIBDIR in config.h.

I decided to go another way... Wanted to make the plugin search work out of
the box. Or do we want to keep that optional?

(AC_DEFINE_UNQUOTED() fails if PLUGINDIR is not defined and you try
${libdir}/openvpn/plugins instead. That evaluates to
"${exec_prefix}/lib/openvpn/plugins" and breaks the path in config.h.)
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpDcQgOS59i6.pgp
Description: OpenPGP digital signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Introduce and use secure_memzero() to erase secrets

2016-11-29 Thread Arne Schwabe


Am 29.11.16 um 11:18 schrieb Gert Doering:
> Hi,
>
> On Tue, Nov 29, 2016 at 11:07:12AM +0100, Arne Schwabe wrote:
>> I think we should prefer memset_s if available since it is a C11 standard.
> Since we're currently compiling against C99, we can't rely on it being
> there (in my OS Zoo, only MacOS seems to have it, at least "have a manpage
> for it" - not even FreeBSD 11 or "gentoo current") - but then, configure 
> could check that for us...
I wouldn't add a configure test for that. According to
http://en.cppreference.com/w/c/string/byte/memset:

As all bounds-checked functions, |memset_s| is only guaranteed to be
available if __STDC_LIB_EXT1__ is defined by the implementation and if
the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before
including |string.h|.

so preprocessor magic can check that.
>
> Shall we merge the current patch anyway, as the memory barrier does the
> job on all current compilers, and add a memset_s() variant "whenever 
> one of us finds time to add and test this"?
>
Yes. Go ahead. I might
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Introduce and use secure_memzero() to erase secrets

2016-11-29 Thread Gert Doering
Hi,

On Tue, Nov 29, 2016 at 11:07:12AM +0100, Arne Schwabe wrote:
> I think we should prefer memset_s if available since it is a C11 standard.

Since we're currently compiling against C99, we can't rely on it being
there (in my OS Zoo, only MacOS seems to have it, at least "have a manpage
for it" - not even FreeBSD 11 or "gentoo current") - but then, configure 
could check that for us...

Shall we merge the current patch anyway, as the memory barrier does the
job on all current compilers, and add a memset_s() variant "whenever 
one of us finds time to add and test this"?

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Introduce and use secure_memzero() to erase secrets

2016-11-29 Thread Steffan Karger
Hi,

On 29-11-16 00:00, Zhaomo Yang wrote:
> Just wanted to let you know that we have a new implementation of
> secure_memzero.h, which is available at 
> https://compsec.sysnet.ucsd.edu/secure_memzero.h
> .
> 
> The version I sent to you guys has a minor issue in dealing with
> memset_s. Also, this implementation synthesizes most of the existing
> working solutions for this problem.  It allows you to specify, via
> macros, which implementation you prefer. This will let users override
> our defaults, if they want.

Thanks, but I think I prefer the more light-weight approach you sent
earlier (slightly molded to use static inline wrapper functions instead
of macro's).  It does what it needs to do for us, and is sufficiently
small to easily integrate into existing code.

And thanks again for notifying us (on security@) on your findings.

-Steffan


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH applied] Re: Map restart signals from event loop to SIGTERM during exit-notification wait

2016-11-29 Thread Gert Doering
ACK, thanks a lot.

I could reproduce the effect as well -- set "--explicit-exit-notify 5",
try to connect to a non-responsive server (unused IP address, ...), and 
~2 seconds before the 60 second timeout, press ctrl-c.  Flooding of
SIGUSR1 messages, no way to stop it (except "kill -9"), quite an annoyance.

With your patch, this changes to

  Tue Nov 29 08:55:19 2016 UDPv4 link remote: [AF_INET]199.102.77.82:51189
  ^C
  Tue Nov 29 08:56:17 2016 event_wait : Interrupted system call (code=4)
  Tue Nov 29 08:56:17 2016 SIGTERM received, sending exit notification to peer
  Tue Nov 29 08:56:19 2016 TLS Error: TLS key negotiation failed to occur 
within 60 seconds (check your network connectivity)
  Tue Nov 29 08:56:19 2016 TLS Error: TLS handshake failed
  Tue Nov 29 08:56:19 2016 Converting soft SIGUSR1 received during exit 
notification to SIGTERM
  Tue Nov 29 08:56:19 2016 SIGTERM[soft,exit-with-notification] received, 
process exiting

.. which fixes the issue for good.

"Real signals" are still ignored (kill -USR1 from another window):

Tue Nov 29 08:58:19 2016 event_wait : Interrupted system call (code=4)
Tue Nov 29 08:58:19 2016 Ignoring SIGUSR1 received during exit notification

(or pressing ctrl-c a second time while it's waiting for the exit notification
to proceed)


(Also, the explanation and the code change make sense :-) )


I've added a blank to the comment line ("true.If" -> "true. If").

Your patch has been applied to the master branch.

commit f25a0217e35f53c3110ebb226e1d1f3528152cb5
Author: Selva Nair
Date:   Mon Nov 28 21:27:04 2016 -0500

 Map restart signals from event loop to SIGTERM during exit-notification 
wait

 Signed-off-by: Selva Nair 
 Acked-by: Gert Doering 
 Message-Id: <1480386424-30876-1-git-send-email-selva.n...@gmail.com>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13284.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel