Re: [Openvpn-devel] 2.4 sees all client certificates as expired when using crl-verify

2017-01-02 Thread SviMik

> On 02-01-17 15:26, Gert Doering wrote:
> > On Mon, Jan 02, 2017 at 03:17:23PM +0100, Alberto Gonzalez Iniesta wrote:
> >> I just got this [1] bug report on OpenVPN 2.4 threating all certs as
> >> expired when upgrading from 2.3. I find this quite weird, but until I have
> >> some time to test it I thought asking here would be faster.
> > 
> > From the bug report:
> > 
> > Mon Jan  2 07:37:10 2017 us=466023 1.2.3.4:36241 VERIFY ERROR: depth=0,
> error=CRL has expired: C=XX, ST=XX, L=XXX, O=None, CN=mycn,
> emailAddress=my@email
> > 
> > "what the log says" :-)
> > 
> > 2.4 checks CRLs much more rigidly than 2.3 (precisely: 2.3 had some
> > built-in checking which only looked at revocations, while 2.4 leaves this 
> > to the crypto library, and they check all fields more rigidly).
> > 
> > Specifically, CRLs with an expired "next update" field are flagged as
> > "expired" by OpenSSL, while the built-in check in 2.3 did not.
> 
> This.  I replied something similar on the debian bug tracker, but I have
> no clue what will happen with that mail.
> 
> > Since this bit a few people already, I wonder how we could communicate
> > this better.
> 
> I wonder about that too.  Maybe some more verbose text on a wiki page?
> We could even detect this specific error and add a link to that page in
> the warning.
> 

Is there an option to disable this check? It would be extremely useful to 
maintain (at least optional) backward compatibility with already existing 
setups, which originally relied on 2.3 behaviour.
--
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/1] add more security features for systemd units

2016-12-12 Thread SviMik
> Hi,
> 
> On Fri, Dec 09, 2016 at 07:13:03PM +0100, Christian Hesse wrote:
> > From: Christian Hesse 
> > 
> > ProtectSystem=strict mounts the entire file system hierarchy read-only,
> > except for the API file system subtrees /dev, /proc and /sys (which can
> > be protected using PrivateDevices=, ProtectKernelTunables=,
> > ProtectControlGroups=).
> 
> Unless the temp directories are still writeable, this will break 
> server configs with --client-connect scripts or plugins trying to hand 
> back config settings via temp files.
> 
> (I do not think an openvpn *client* config will need a to create
> files, but this needs testing)
> 

Even if you find a way to store temporary files, I'm still not sure what can be 
done with ifconfig-pool-persist. It's not a temp file, it should be persistent.
--
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/1] add more security features for systemd units

2016-12-09 Thread SviMik
> You can break this with something like:
> 
> status /etc/openvpn/client/status.log
> 
> in your configuration. Writing a status file
> to /run/openvpn-{client,server}/status.log works, though. So the default
> setups should be fine. Do we have any more cases where openvpn wants write
> access for whatever?

>From my configuration:
1) status
2) ifconfig-pool-persist
3) tmp-dir (for storing openvpn_pf_*.tmp files)
4) client-connect script may want to write something
5) a plugin may want to write something

For me even the read-only option will break nearly *everything*. And for user 
it will be completely not obvious why his scripts doesn't work, why his status 
file is not updated, and what's wrong with ifconfig-pool-persist.
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] push: Provide a warning if --ifconfig-push have argument mismatch with --topology

2016-11-30 Thread SviMik
While I admit that it is *extremely* unlikely to have a network larger than /8, 
such logic still looks a little clumsy. It does not cover all the valid 
netmasks neither it detects all possible invalid ones.

If you wish to test if the netmask is valid, this solution could be better:
http://stackoverflow.com/questions/17401067/c-code-for-valid-netmask


> This adds a warning to the log file if --topology is configured to use
> subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option
> is not an subnet mask.  The check done is to ensure the first octet is 0xff
> (255)
> 
> Trac: #755
> Signed-off-by: David Sommerseth 
> ---
>  src/openvpn/push.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 9953079..0819bf8 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -333,6 +333,16 @@ prepare_push_reply (struct context *c, struct gc_arena
> *gc,
>  print_in_addr_t (ifconfig_local, 0, gc),
>  print_in_addr_t (c->c2.push_ifconfig_remote_netmask,
>   0, gc));
> +
> +  /* Warn if ifconfig_remote_netmask does not contain
> +   * subnet mask in subnet topology */
> +  if ((c->options.topology == TOP_SUBNET || c->options.topology ==
> TOP_NET30)
> +  && (c->c2.push_ifconfig_remote_netmask & 0xff00) != 0xff00)
> +{
> +  msg(M_WARN, "WARNING: When --topology is subnet or net30, the
> second argument "
> +  "to --ifconfig-push MUST be a subnet mask.  This client will
> not be able "
> +  "to connect.");
> +}
>  }
>  
>/* Send peer-id if client supports it */
> -- 
> 1.8.3.1
> 
> 
> --
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 
--
___
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-30 Thread SviMik
1) I would also check if the file size was changed, not only mtime.

2) I wasn't digging the code deeply, but the
> ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime
makes me think it would fail if the file goes reverted to a previous version. 
Perhaps the check shall be != instead of >=.


> 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)
> +{
> +  /* 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 is the primary routine for processing TLS stuff inside the
>   * the main event loop.  When this routine exits
> @@ -2581,12 +2612,15 @@ tls_process (struct tls_multi *multi,
> ks->state = S_START;
> state_change = true;
>  
> -   /* Reload the CRL before TLS negotiation */
> +   /*
> +* Attempt CRL reload before TLS negotiation. Won't be performed if
> +* the file was not modified since the last reload
> +*/
> if (session->opt->crl_file &&
> !(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))
>   {
> tls_ctx_reload_crl(>opt->ssl_ctx,
> -   session->opt->crl_file, session->opt->crl_file_inline);
> +   session->opt->crl_file, session->opt->crl_file_inline, true);
>   }
>  
> dmsg (D_TLS_DEBUG_MED, "STATE S_START");
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index 0777c61..3fbd2b4 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -353,7 +353,7 @@ void key_state_ssl_free(struct key_state_ssl *ks_ssl);
>   *  "[[INLINE]]" in the case of inline files.
>   * @param crl_inlineA string containing the CRL
>   */
> -void tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
> +void backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
>  const char *crl_file, const char *crl_inline);
>  
>  /**
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 7fa35a7..11ee65b 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -771,7 +771,7 @@ static void tls_version_to_major_minor(int tls_ver, int
> *major, int *minor) {
>  }
>  
>  void
> -tls_ctx_reload_crl(struct tls_root_ctx *ctx, const char *crl_file,
> +backend_tls_ctx_reload_crl(struct tls_root_ctx *ctx, const 

[Openvpn-devel] [PATCH] Fix using a pointer before checking against null

2016-11-24 Thread SviMik
There was a few places where pointer was used and then checked
against null.

This patch is trying to fix that by adding additional checks or moving
it into the right place.

Signed-off-by: Sviatoslav Mikhailov 
---
 src/openvpn/buffer.c |2 +-
 src/openvpn/ps.c |4 ++--
 src/openvpn/push.c   |2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 52c6ab9..57956dd 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -439,7 +439,7 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput,
   struct gc_arena *gc)
 {
   struct buffer out = alloc_buf_gc (maxoutput ? maxoutput :
-   ((size * 2) + (size / (space_break_flags & 
FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2),
+   ((size * 2) + (size / (space_break_flags & 
FHE_SPACE_BREAK_MASK)) * (separator ? (int) strlen (separator) : 0) + 2),
gc);
   int i;
   for (i = 0; i < size; ++i)
diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index 2cb68f1..935bd0b 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -499,6 +499,7 @@ control_message_from_parent (const socket_descriptor_t 
sd_control,
   mesg.msg_flags = 0;
 
   h = CMSG_FIRSTHDR();
+  ASSERT (h);
   h->cmsg_len = CMSG_LEN(sizeof(socket_descriptor_t));
   h->cmsg_level = SOL_SOCKET;
   h->cmsg_type = SCM_RIGHTS;
@@ -508,8 +509,7 @@ control_message_from_parent (const socket_descriptor_t 
sd_control,
   status = recvmsg (sd_control, , MSG_NOSIGNAL);
   if (status != -1)
 {
-  if (   h == NULL
- || h->cmsg_len!= CMSG_LEN(sizeof(socket_descriptor_t))
+  if ( h->cmsg_len!= CMSG_LEN(sizeof(socket_descriptor_t))
  || h->cmsg_level  != SOL_SOCKET
  || h->cmsg_type   != SCM_RIGHTS )
{
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 9953079..b016d99 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -565,7 +565,7 @@ push_remove_option (struct options *o, const char *p)
   msg (D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
   /* ifconfig-ipv6 is special, as not part of the push list */
-  if ( streq( p, "ifconfig-ipv6" ))
+  if (o && streq( p, "ifconfig-ipv6" ))
 {
   o->push_ifconfig_ipv6_blocked = true;
   return;
-- 
1.7.10.msysgit.1
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] Proper location for user-specific OpenVPN configuration files used by OpenVPN-GUI?

2016-10-19 Thread SviMik
> Maybe add an 'open config folder' button/link to openvpn-gui?
Sounds like a good idea in any case!
Why it hasn't been done, like, 10 years ago? :D I spent sooo much time opening 
the C:\Program Files\OpenVPN\config !

But my vote is still for %USERPROFILE%.

--
SviMik
--
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] Proper location for user-specific OpenVPN configuration files used by OpenVPN-GUI?

2016-10-19 Thread SviMik
> Three directory alternatives have been proposed:
> 
> %USERPROFILE%\OpenVPN\config (current approach)
> %APPDATA%\OpenVPN\config
> %USERPROFILE%\.openvpn\config
> 

I vote for %USERPROFILE%, as it is more intuitive.

%APPDATA% seems to be a folder for a files generated solely by software, and 
not for things which user may want to put manually like VPN configs. I have 
just checked - %APPDATA% is even a hidden folder on my machine. And yes, I 
don't trust the import function, cause I don't know what it will do in some 
situations, especially when certs are stored in external files, or there are 
up\down scripts that should also be copied, or...

".openvpn" - definitely NO. It looks just ugly. Even Linux doesn't use that for 
configuration folders (remember /etc/openvpn?).

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