Re: [Openvpn-devel] ASLR/DEP -enabled 2.4.0 Windows installer available

[Selva Nair]

Hi,

On Mon, Jan 23, 2017 at 4:55 AM, Samuli Seppänen  wrote:

>
>> Checked this on win7. Process explorer shows ASLR flag is set on the
>> executable. But ASLR is not really active. The GUI is loaded at the same
>> address each time (as per vmmap from sysinternals). I see no address
>> randomization.
>>
>> Recompiling by exporting at least one function fixes this so the linker
>> is indeed not adding reloc section to the exe otherwise.
>>
>> The build option does make ASLR work for openssl dll so the only thing
>> missing there was the flag in the header. Not so for the executables.
>>
>> We need to find some fix for this, else I fear this will be ASLR in name
>> only.
>>
>> Selva
>>
>
> So we need a small code change in OpenVPN to get ASLR actually working,
> like in OpenVPN GUI?
>
> 
>
> In particular something like this:
>
>  2045016cb90d1e65d71c2407a2570927R72>
>
> Correct?


There are many suggestions online including add --export-all-symbols to
LDFLAGS, mark main() with dllexport etc. I thought --export-all-symbols is
an overkill and suggested to just export a dummy variable for the GUI.

I don't fully understand how ASLR is implemented on Windows but my tests
show code, stack, heap and PEB addresses are randomized if dynamicbase flag
is on and there is a .reloc section in the exec (ld appears to add this
only if something is exported). So I can only say this works, not sure its
the best workaround.

I wrote a small program to see ASLR in action: it prints some
representative pointers to code, stack, heap etc to see how they change
between runs. The program and test results are here <
https://gist.github.com/selvanair/c8ffa0fe60710e05c0e38f7d0097468d>

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 v2] Fix user's group membership check in interactive service to work with domains

[Selva Nair]

Hi,

On Sat, Jan 14, 2017 at 4:16 PM,  wrote:

> From: Selva Nair 
>
> Currently the username unqualified by the domain is used to validate
> a user which fails for domain users. Instead authorize the user
>
> (i) if the built-in admin group or ovpn_admin group is in the process token
> (ii) else if the user's SID is in the built-in admin or ovpn_admin groups
>
> The second check is needed to recognize dynamic updates to group membership
> on the local machine that will not be reflected in the token.
>
> These checks do not require connection to a domain controller and will
> work even when user is logged in with cached credentials.
>
> Resolves Trac: #810
>
> v2: include the token check as described above


Bump :) This addresses a critical issue that I would like to see fixed in
2.4.1..

Thanks,

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] Resolving several travis-ci issues:

[Selva Nair]

On Fri, Jan 27, 2017 at 10:08 AM, David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 27/01/17 14:56, Илья Шипицин wrote:
> >
>
> >
> > may I ask you something in turn ?
> > I cannot read other people thoughts, if there's something wrong with my
> > patch, there's no other known way, but your reply.
> >
> > since, you are keeping silence, I've no idea whether is it wrong or not.
> > It would be really nice if you will tell "that and that is wrong". I'm
> > sure, there are sites about git, where it is written.
>
> Fair point, and this is a very common issue in most open source
> projects.  We are not necessarily ignoring you just for the fun of it.
> But we do look through a lot of patches, and need to prioritise them.
> For example: Patches which fixes real issues in released OpenVPN
> versions naturally gets a higher priority than a patch fixing a man page.


That is hardly true. I am not complaining, but just pointing out some
reality that can't be helped.

We have a dozen man page or similar level improvements since 2.4.0 though
at least one critical bug fix I submitted is still awaiting review. This
has to be expected as it takes the right person and quality time to get
certain things reviewed.  Sometimes trivialities like the non-exiting
inefficiency (save a microsecond once an hour?)  of carrying around
[[INLINE]] tag may get multiple reviews, but a bug fix may wait for ever
and even lost under the pile.

It all depends what catches our fancy, personal interest, expertise and
mood. No point in trying to rationalize it beyond that.

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] Resolve several travis-ci issues

[Selva Nair]

Hi,

On Sat, Jan 28, 2017 at 3:06 AM, Ilya Shipitsin 
wrote:

> MBEDTLS_VERSION, OPENSSL_VERSION were defined twice - in both
>  .travis.yml  and .travis/build-deps.sh files, the last one
> defined OPENSSL_VERSION via nonexistent OPENSSL_VERION
> variable, which lead us to use openssl-1.0.1 instead of
> openssl-1.0.2, I removed variable definition from build-deps.sh
>
> "cache: [ apt: true ]" is not a travis supported option, it was
> introduced by mistake, I removed it
>
> LD_LIBRARY_PATH was defined for the entire test run, it includes
> custom openssl build, which was picked by "wget", so "wget"
> could not verify SSL cert at https://www.openssl.org sometimes.
> We do not want wget to pick our custom LD_LIBRARY_PATH, so I moved
> that variable to "script" section
>
> LD_LIBRARY_PATH was defined for both linux and osx environments,
> for the second DYLD_LIBRARY_PATH must be defined instead
>
> v2: Upgrade openssl, mbedtls to the most recent versions
> ---
>  .travis.yml   | 8 
>  .travis/build-deps.sh | 2 --
>  2 files changed, 4 insertions(+), 6 deletions(-)
>

Moving down the LD_LIBRARY_PATH definition makes sense -- we need it (more
on that below) in the env  to run the test executables, but do not want it
to modify the library search path for installed programs used for package
updates etc.

 script:
> +  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export
> LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi
> +  - if [ "${TRAVIS_OS_NAME}" = "osx"   ]; then export
> DYLD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi


On this platform DYLD_LIBRARY_PATH is what matters, so I think we should be
appending to it instead of redefining it in terms of LD_LIBRARY_PATH. That
is,

DYLD_LIBRARY_PATH="${PREFIX}/lib:${DYLD_LIBRARY_PATH}"

While this patch is good enough for travis builds,  there is a more general
issue here. I think its the job of libtool to set -rpath properly (and/or
use a wrapper) so that uninstalled test programs pick up the library it was
linked with. Else a developer building using custom libs gets wrong
libraries loaded while running tests. A proper fix will eliminate the need
for tweaking LD_LIBRARY_PATH in the first place. But my libtool foo is
virtually nil to suggest a proper solution.

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] Resolve several travis-ci issues

[Selva Nair]

On Sun, Jan 29, 2017 at 1:58 AM, Ilya Shipitsin 
wrote:

> MBEDTLS_VERSION, OPENSSL_VERSION were defined twice - in both
> .travis.yml  and .travis/build-deps.sh files, the last one
> defined OPENSSL_VERSION via nonexistent OPENSSL_VERION
> variable, which lead us to use openssl-1.0.1 instead of
> openssl-1.0.2, I removed variable definition from build-deps.sh
>
> "cache: [ apt: true ]" is not a travis supported option, it was
> introduced by mistake, I removed it
>
> LD_LIBRARY_PATH was defined for the entire test run, it includes
> custom openssl build, which was picked by "wget", so "wget"
> could not verify SSL cert at https://www.openssl.org sometimes.
> We do not want wget to pick our custom LD_LIBRARY_PATH, so I moved
> that variable to "script" section
>
> LD_LIBRARY_PATH was defined for both linux and osx environments,
> for the second DYLD_LIBRARY_PATH must be defined instead
>
> v2: Upgrade openssl, mbedtls to the most recent versions
>
> v3: DYLD_LIBRARY_PATH was defined via LD_LIBRARY_PATH by mistake


This should take care of travis builds so ACK from me.

For completeness I will copy what I wrote before:
While this patch is good enough for travis builds,  there is a more general
issue here. I think its the job of libtool to set -rpath properly (and/or
use a wrapper) so that uninstalled test programs pick up the library it was
linked with.

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] reload HTTP proxy credentials when moving to the next connection profile

[Selva Nair]

Hi,

On Tue, Jan 31, 2017 at 1:22 PM, Antonio Quartulli  wrote:

> iff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c
> index b0ed3279..27f34bed 100644
> --- a/src/openvpn/proxy.c
> +++ b/src/openvpn/proxy.c
> @@ -256,7 +256,16 @@ username_password_as_base64(const struct
> http_proxy_info *p,
>  static void
>  get_user_pass_http(struct http_proxy_info *p, const bool force)
>  {
> -if (!static_proxy_user_pass.defined || force)
> +/*
> + * in case of forced (re)load, make sure the static storage is set as
> + * undefined, otherwise get_user_pass() won't try to load any
> credential
> + */
> +if (force)
> +{
> +static_proxy_user_pass.defined = false;
> +}
>

While unsetting the define attribute appears to have no unintended side
effects,  purge_user_pass(&static_proxy_user_pass, true) would be cleaner.


> +
> +if (!static_proxy_user_pass.defined)
>  {
>  unsigned int flags = GET_USER_PASS_MANAGEMENT;
>  if (p->queried_creds)
>

That said, there is one issue with this approach. Looks like SIGUSR1
restarts will now always prompt for proxy password, which is not proper. If
so, a more nuanced fix that preserves the current behaviour for a single
proxy credentials is required. May be, do the "purge proxy pass" in init.c
only if the connection entry has http-proxy specified?

I think the intent of the original code/man page is to have proxy password
work the same way as auth-user-pass with an implicit auth-nocache assumed.
Then multiple credentials is not expected, but could still work if the
proxy password is purged when an authentication error happens instead of
when the remote changes.

Finally, socks credentials may also be affected.

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] reload HTTP proxy credentials when moving to the next connection profile

[Selva Nair]

On Wed, Feb 1, 2017 at 3:33 AM, Antonio Quartulli  wrote:

> On Wed, Feb 01, 2017 at 11:04:55AM +0800, Antonio Quartulli wrote:
> > > That said, there is one issue with this approach. Looks like SIGUSR1
> > > restarts will now always prompt for proxy password, which is not
> proper.
> >
> > Right! Thanks for pointing this out!
>
> Actually I'd like to understand the expected behaviour: if SIGUSR1 is
> issued,
> openvpn should re-read the file containing the HTTP proxy credentials,
> but, if
> stdin was specified, it should not ask the user for user/pass again ?
>
> So in case of stdin the only way to re-enter the user/pass is to restart
> openvpn?
>
> What's your opinion on this?


I wrote SIGUSR1 but its the same with SIGHUP. All [*] passwords are cached
by default and http-proxy password is always cached (auth-nocache or not).
So the expected behaviour is password to be read only once during the
lifetime of the process unless an auth error happens.

The source of the password does not matter (stdin, systemd, file, mgmt,
inline et..), its always read using get_user_pass which gets it afresh if
no cached copy is available.

With only one cache per password type, this expects passwords are not
remote-specific. As we support only one auth-user-pass or one private key
pass per config I think the intent was to support only one proxy password.
Allowing http-proxy in  block gives a different impression,
though..

Selva

[*] Not sure about socks password
--
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] Make auth-nocache cooperate with auth-token

[selva . nair]

From: Selva Nair 

- Keep the username even if auth-nocache is specified so that
  any auth_token pushed by the server could be utilized
- When auth-token is received, set nocache = false in user_pass

Note: When handling of auth failure due to token expiry is fixed, remember
to re-instate nocache after clearing the token

Trac: #840

Signed-off-by: Selva Nair 
---
 src/openvpn/misc.c | 6 --
 src/openvpn/ssl.c  | 4 
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index a2f45b6..5954eea 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -1490,10 +1490,12 @@ purge_user_pass(struct user_pass *up, const bool force)
 void
 set_auth_token(struct user_pass *up, const char *token)
 {
-if (token && strlen(token) && up && up->defined && !up->nocache)
+if (token && strlen(token) && up && strlen(up->username))
 {
-CLEAR(up->password);
+secure_memzero(up->password, sizeof(up->password));
 strncpynt(up->password, token, USER_PASS_LEN);
+up->nocache = false;
+up->defined = true;
 }
 }
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 86450fe..e25e99d 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2383,7 +2383,11 @@ key_method_2_write(struct buffer *buf, struct 
tls_session *session)
 {
 goto error;
 }
+/* keep username for use with any auth-token pushed by the server */
+char username[USER_PASS_LEN];
+strncpynt(username, auth_user_pass.username, sizeof(username));
 purge_user_pass(&auth_user_pass, false);
+strncpynt(auth_user_pass.username, username, 
sizeof(auth_user_pass.username));
 }
 else
 {
-- 
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] Make auth-nocache cooperate with auth-token

[Selva Nair]

Hi,

On Wed, Feb 8, 2017 at 10:01 PM, Antonio Quartulli  wrote:

> On Wed, Feb 08, 2017 at 02:25:44PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - Keep the username even if auth-nocache is specified so that
> >   any auth_token pushed by the server could be utilized
>
> This means that even when using no auth-token the username will be cached.
> Can this be a security concern?
>

I would consider username as not sensitive  material although not sure
everyone would agree. Unfortunately there is no way to know in advance that
auth-token may get pushed so I can't think of a good way of avoiding this.
A not so secure approach (I considered this first) would be to delay
clearing the username/password to post pushed-options processing, but then
one has to handle cases like what if the push reply never arrives and so
on.. In general its always better to clear sensitive data at the earliest.

The way out would be to do one more purge_user_pass(.., false) after push
processing.. sigh..  will go there only if absolutely necessary.

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] Fix building with LibreSSL 2.5.1 by cleaning a hack.

[Selva Nair]

Hi,

On Mon, Feb 13, 2017 at 3:55 PM, Olivier W  wrote:

> >> That's a not exactly helpful error message... :( - I tend to just turn
> >> off SSL on stuff that goes to public mailing lists anyway if it causes
> >> issues...
> >
> > OpenSSL errors requires quite some efforts to get used to.  And in
> > addition the git-send-email errors on top doesn't always make life
> easier.
>
> I've just tried git-send-email with "--smtp-debug=1" and the error
> isn't much useful, I'm getting:
> "...
> Net::SMTP=GLOB(0x8048189a8)<<< 250 SMTPUTF8
> Net::SMTP=GLOB(0x8048189a8)>>> STARTTLS
> Net::SMTP=GLOB(0x8048189a8)<<< 220 2.0.0 Ready to start TLS
> Net::SMTP=GLOB(0x8048189a8)>>> STARTTLS
> Net::SMTP: Net::Cmd::getline(): unexpected EOF on command channel:
> Connection reset by peer at /usr/local/libexec/git-core/git-send-email
> line 1371.
> STARTTLS failed!  at /usr/local/libexec/git-core/git-send-email line
> 1371."
>

On Debian jessie, the following .gitconfig works fo me.

[sendemail]
  smtpEncryption = tls
  smtpServer = smtp.gmail.com
  smtpUser = user.n...@gmail.com 
  smtpServerPort = 587

No smtpsslcertpath specified, I suppose it verifies the cert using
/etc/ssl/certs as the capath, which is the default.

Possibly your /etc/ssl/cert.pem is to blame? I do not have such a file, so
no idea what it contains.

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 v2] Fix user's group membership check in interactive service to work with domains

[Selva Nair]

On Mon, Feb 20, 2017 at 7:18 AM, Gert Doering  wrote:

> On Sat, Jan 14, 2017 at 04:16:29PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > Currently the username unqualified by the domain is used to validate
> > a user which fails for domain users. Instead authorize the user
> >
> > (i) if the built-in admin group or ovpn_admin group is in the process
> token
> > (ii) else if the user's SID is in the built-in admin or ovpn_admin groups
> >
> > The second check is needed to recognize dynamic updates to group
> membership
> > on the local machine that will not be reflected in the token.
> >
> > These checks do not require connection to a domain controller and will
> > work even when user is logged in with cached credentials.
> >
> > Resolves Trac: #810
> >
> > v2: include the token check as described above
>
> Took me way too long...  the code change looks reasonable ("does what it
> says on the tin, and safely so").
>
> One questions occured to me, though...
>
> MS documentation for GetTokenInformation() suggests that group membership
> tests should be done with "CheckTokenMembership()", which sounds more
> convenient than "extract them all and walk the list" - so maybe this
> is done to avoid domain controller contact?


Thanks for the review :)

CheckTokenMembership() returns true only if the SID is present and enabled.
That means when UAC is active it will not detect that the user is a member
of administrators group as the SID will not be enabled. In other words, our
usage of group membership is somewhat special -- we only care user is a
member of admin or ovpn_admin groups, not that the corresponding rights be
enabled in the token.

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] why "ifconfig" is required during build ?

[Selva Nair]

Hi,

On Wed, Mar 15, 2017 at 11:37 AM, Илья Шипицин  wrote:

> >
>> > well, it makes sense that it might be required for running openvpn. but
>> why
>> > to check it during build ?
>>
>> To find the path that we're going to call the binary with.  We do not
>> rely on $PATH resolution at runtime.
>>
>
> well, what if I will not change current (i.e. default behaviour), but
> instead I will add a possibility to specify ifconfig explicitly
>

You can set custom path during configure like this:

$ IFCONFIG=/opt/hacks/bin/ifconfig ./configure

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] Fix installation of IPv6 host route to VPN server when using iservice.

[Selva Nair]

Hi,

On Sun, Mar 19, 2017 at 3:10 PM, Gert Doering  wrote:

> The "prepare IPv6 route message to interactive service" was properly
> handing the correct interface index (r->adapter_index) for this case,
> but then always overwrote the gateway address with our magic tun/tap
> fe80::8 value.  Only do this for "on tap adapter" routes.
>
> Pinpointed by Selva Nair.
>
> Trac #850
>
> Signed-off-by: Gert Doering 
> ---
>  src/openvpn/route.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 7e536ef..08998d5 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -3061,8 +3061,10 @@ do_route_ipv6_service(const bool add, const struct
> route_ipv6 *r, const struct t
>
>  /* In TUN mode we use a special link-local address as the next hop.
>   * The tapdrvr knows about it and will answer neighbor discovery
> packets.
> + * (only do this for routes actually using the tun/tap device)
>   */
> -if (tt->type == DEV_TYPE_TUN)
> +if (tt->type == DEV_TYPE_TUN
> +&& msg.iface.index == tt->adapter_index )
>
 {
>  inet_pton(AF_INET6, "fe80::8", &msg.gateway.ipv6);
>  }
>

This should fix it. ACK.

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] Upgrading EasyRSA 2's defaults

[Selva Nair]

On Mon, Apr 3, 2017 at 4:43 PM, David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 03/04/17 16:12, Jan Just Keijser wrote:
> > Hi Samuli,
> >
> > On 03/04/17 15:53, Samuli Seppänen wrote:
> >> On 02/04/2017 10:57, Steffan Karger wrote:
>

snip..


> >>> DSA is _not_ a preferred choice.  The original 1024-bit DSA is too weak
> >>> nowadays, and the 'larger' DSA variants are not even close to the wide
> >>> support that RSA has.
> >>>
> >>> -Steffan
> >>>
> >> Hi,
> >>
> >> I've issue a pull request here and review would be appreciated:
> >>
> >> 
> >>
> >> I tested these changes on Debian 8 which has OpenSSL-1.0.1. Key size was
> >> set to 4096-bits and signature algorithm to SHA256WithRSAEncryption.
> >>
> >> The only real issue was DH parameter generation: it took ~25 minutes on
> >> my Intel i5 laptop. Is that acceptable default behavior?
> >>
> > what kind of i5 is this? on my i7-4810 it took 5 minutes. Can you give
> the full CPUID string (from /proc/cpuinfo) ?  then I can
> > guestimate whether the 25 minutes is realistic for slower hardware.
>
> I've run a a couple of "quick" tests ... on a two different laptops
>
> --- test 1 --
> $ time openssl gendh -out test 4096
> [...snip...]
>
> real35m40.098s
> user35m38.922s
> sys 0m0.367s
> $ cat /proc/cpuinfo  | grep model\ name | uniq -c
>   4 model name  : Intel(R) Core(TM) i5-6200U CPU @ 2.30GHz
>

4096 bit "strong" prime is indeed an intensive computation.. Is using
-dsaparam  option not secure enough?

openssl dhparam -dsaparam -out test 4096

is 15 seconds vs forever without it on my ancient desktop.

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] Upgrading EasyRSA 2's defaults

[Selva Nair]

Hi,

On Tue, Apr 4, 2017 at 3:48 AM, Steffan Karger  wrote:

> From the openssl man page:
>
> "Beware that with such DSA-style DH parameters, a fresh DH key should
> be created for each use to avoid small-subgroup attacks that may be
> possible otherwise."
>
> This means that if for some reason a non-ephemeral diffie-hellman
> cipher suite is selected, you are at risk of these attacks.
>

Thanks for the clarification.

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 v2] Delete the IPv6 route to the "connected" network on tun close

[Selva Nair]

While cleaning up my local branches this one came up..

Any comments? -- a NAK will do as well so that I can delete it :)

Selva

On Fri, Nov 25, 2016 at 12:21 AM, Selva Nair  wrote:

> This was missing on Windows when interactive service is in use.
>
> - Added route_ipv6_clear_host_bits(r6) to delete_route_ipv6: this is
>   required for Windows IP-helper API. Won't hurt other platforms (?)
>
> v2: Be const correct: route in delete_route_ipv6() made non-const.
> None of the exisitng calls are affected.
>
> Signed-off-by: Selva Nair 
> ---
>  src/openvpn/route.c | 4 +++-
>  src/openvpn/route.h | 2 +-
>  src/openvpn/tun.c   | 3 +++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index fec12c1..34b1196 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2102,7 +2102,7 @@ delete_route (struct route_ipv4 *r,
>  }
>
>  void
> -delete_route_ipv6 (const struct route_ipv6 *r6, const struct tuntap *tt,
> unsigned int flags, const struct env_set *es)
> +delete_route_ipv6 (struct route_ipv6 *r6, const struct tuntap *tt,
> unsigned int flags, const struct env_set *es)
>  {
>struct gc_arena gc;
>struct argv argv = argv_new ();
> @@ -2124,6 +2124,8 @@ delete_route_ipv6 (const struct route_ipv6 *r6,
> const struct tuntap *tt, unsigne
>
>gc_init (&gc);
>
> +  route_ipv6_clear_host_bits (r6);
> +
>network = print_in6_addr( r6->network, 0, &gc);
>gateway = print_in6_addr( r6->gateway, 0, &gc);
>
> diff --git a/src/openvpn/route.h b/src/openvpn/route.h
> index c358681..70aeb65 100644
> --- a/src/openvpn/route.h
> +++ b/src/openvpn/route.h
> @@ -252,7 +252,7 @@ void copy_route_ipv6_option_list (struct
> route_ipv6_option_list *dest,
>struct gc_arena *a);
>
>  void add_route_ipv6 (struct route_ipv6 *r, const struct tuntap *tt,
> unsigned int flags, const struct env_set *es);
> -void delete_route_ipv6 (const struct route_ipv6 *r, const struct tuntap
> *tt, unsigned int flags, const struct env_set *es);
> +void delete_route_ipv6 (struct route_ipv6 *r, const struct tuntap *tt,
> unsigned int flags, const struct env_set *es);
>
>  void add_route (struct route_ipv4 *r,
> const struct tuntap *tt,
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 560b1a8..40ce202 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -5663,6 +5663,9 @@ close_tun (struct tuntap *tt)
>  {
>if (tt->options.msg_channel)
>  {
> +  /* remove route pointing to interface */
> +  delete_route_connected_v6_net(tt, NULL);
> +
>do_address_service (false, AF_INET6, tt);
>   if (tt->options.dns6_len > 0)
>   do_dns6_service (false, tt);
> --
> 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] devel mailing list

[Selva Nair]

I did not get this mail

https://sourceforge.net/p/openvpn/mailman/message/35789733/

Something up with the list or is it only me?

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] devel mailing list

[Selva Nair]

On Sat, Apr 15, 2017 at 5:17 PM, ValdikSS  wrote:

> Should I try to re-post it? Could it be because of 7z archive?


Possibly gmail blocked it in my case -- I thought 7z will be blocked only
if contained an executable (.exe, .bat etc..)

Please do post again -- the registry entry may be added as a foot note in
plain text ?

Thanks,

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] Windows 10 Creators Update broke --block-outside-dns

[Selva Nair]

On Tue, Apr 25, 2017 at 4:40 PM, ValdikSS  wrote:

> Please check updated version
> https://github.com/ValdikSS/openvpn-with-patches/commit/
> 80345eac823326299c5428a8db45dc06a8d10f7b
>
> set_interface_metric() needs to be called from interactive service but the
> service doesn't include win32.h/c so I had to copy/paste code into it. How
> could this be improved?
>

Code shared between to openvpn core and the service could go into
block_dns.c and block_dns.h. Do not make the service code dependent on
win32.h as that will pull-in a lot of unwanted dependencies with it.

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

[Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[selva . nair]

From: Selva Nair 

If static challenge is in use, the password passed to the plugin by openvpn
is of the form "SCRV1:base64-pass:base64-response". Parse this string to
separate it into password and response and use them to respond to queries
in the pam conversation function.

On the plugin parameters line the substitution keyword for the static
challenge response is "OTP". For example, for pam config named "test" that
prompts for "user", "password" and "pin", use

plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"

Signed-off-by: Selva Nair 
---
 src/plugins/auth-pam/Makefile.am |  3 ++
 src/plugins/auth-pam/README.auth-pam | 15 +---
 src/plugins/auth-pam/auth-pam.c  | 75 
 3 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/src/plugins/auth-pam/Makefile.am b/src/plugins/auth-pam/Makefile.am
index e6dc27e..8563fd4 100644
--- a/src/plugins/auth-pam/Makefile.am
+++ b/src/plugins/auth-pam/Makefile.am
@@ -9,6 +9,8 @@ MAINTAINERCLEANFILES = \
 
 AM_CFLAGS = \
-I$(top_srcdir)/include \
+   -I$(top_srcdir)/src/openvpn \
+   -I$(top_srcdir)/src/compat \
$(PLUGIN_AUTH_PAM_CFLAGS) \
$(OPTIONAL_CRYPTO_CFLAGS)
 
@@ -21,6 +23,7 @@ openvpn_plugin_auth_pam_la_SOURCES = \
utils.c \
auth-pam.c \
pamdl.c  pamdl.h \
+   $(top_srcdir)/src/openvpn/base64.c $(top_srcdir)/src/openvpn/base64.h \
auth-pam.exports
 openvpn_plugin_auth_pam_la_LIBADD = \
$(PLUGIN_AUTH_PAM_LIBS)
diff --git a/src/plugins/auth-pam/README.auth-pam 
b/src/plugins/auth-pam/README.auth-pam
index e123690..9081565 100644
--- a/src/plugins/auth-pam/README.auth-pam
+++ b/src/plugins/auth-pam/README.auth-pam
@@ -36,19 +36,20 @@ pairs to answer PAM module queries.
 
 For example:
 
-  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD"
+  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD pin OTP"
 
 tells auth-pam to (a) use the "login" PAM module, (b) answer a
-"login" query with the username given by the OpenVPN client, and
-(c) answer a "password" query with the password given by the
-OpenVPN client.  This provides flexibility in dealing with the different
+"login" query with the username given by the OpenVPN client,
+(c) answer a "password" query with the password, and (d) answer a
+"pin" query with the OTP given by the OpenVPN client.
+This provides flexibility in dealing with different
 types of query strings which different PAM modules might generate.
 For example, suppose you were using a PAM module called
 "test" which queried for "name" rather than "login":
 
   plugin openvpn-auth-pam.so "test name USERNAME password PASSWORD"
 
-While "USERNAME" "COMMONNAME" and "PASSWORD" are special strings which 
substitute
+While "USERNAME" "COMMONNAME" "PASSWORD" and "OTP" are special strings which 
substitute
 to client-supplied values, it is also possible to name literal values
 to use as PAM module query responses.  For example, suppose that the
 login module queried for a third parameter, "domain" which
@@ -61,6 +62,10 @@ the operation of this plugin:
 
   client-cert-not-required
   username-as-common-name
+  static-challenge
+
+Use of --static challenege is required to pass a pin (represented by "OTP" in
+parameter substituion) or a second password.
 
 Run OpenVPN with --verb 7 or higher to get debugging output from
 this plugin, including the list of queries presented by the
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 10622fd..f037eef 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -48,6 +48,7 @@
 #include 
 #include 
 #include "utils.h"
+#include 
 
 #include 
 
@@ -85,6 +86,7 @@ struct auth_pam_context
  *  "USERNAME" -- substitute client-supplied username
  *  "PASSWORD" -- substitute client-specified password
  *  "COMMONNAME" -- substitute client certificate common name
+ *  "OTP" -- substitute static challenege response if available
  */
 
 #define N_NAME_VALUE 16
@@ -109,6 +111,7 @@ struct user_pass {
 char username[128];
 char password[128];
 char common_name[128];
+char response[128];
 
 const struct name_value_list *name_value_list;
 };
@@ -274,6 +277,69 @@ name_value_match(const char *query, const char *match)
 return strncasecmp(match, query, strlen(match)) == 0;
 }
 
+/*
+ * Split and decode up->password in the form SCRV1:base64_pass:base64_response
+ * into pass and response and save in up->password and up->response.
+ * If the password is not in the expected format, input is not changed.
+ */
+static void
+spli

[Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[selva . nair]

From: Selva Nair 

This adds a minimal secure_memzero()

Signed-off-by: Selva Nair 
---
 src/plugins/auth-pam/auth-pam.c |  2 ++
 src/plugins/auth-pam/utils.h| 16 
 2 files changed, 18 insertions(+)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..10622fd 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -799,8 +799,10 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 command);
 goto done;
 }
+secure_memzero(up.password, sizeof(up.password));
 }
 done:
+secure_memzero(up.password, sizeof(up.password));
 
 #ifdef USE_PAM_DLOPEN
 dlclose_pam();
diff --git a/src/plugins/auth-pam/utils.h b/src/plugins/auth-pam/utils.h
index fbc9705..c1fa3ee 100644
--- a/src/plugins/auth-pam/utils.h
+++ b/src/plugins/auth-pam/utils.h
@@ -63,4 +63,20 @@ get_env(const char *name, const char *envp[]);
 int
 string_array_len(const char *array[]);
 
+/**
+ * Securely zero memory without letting optimized away by the compiler
+ *
+ * @param data  Pointer to data to fill with zero
+ * @param len   Length of data, in bytes.
+ */
+inline void
+secure_memzero(void *data, size_t len)
+{
+volatile char *p = (volatile char *) data;
+while (len--)
+{
+*p++ = 0;
+}
+}
+
 #endif
-- 
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 1/2] In auth-pam plugin clear the password after use

[Selva Nair]

On Fri, May 5, 2017 at 3:01 PM, David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 05/05/17 20:28, Gert Doering wrote:
> > Hi,
> >
> > On Fri, May 05, 2017 at 02:24:01PM -0400, selva.n...@gmail.com wrote:
> >> From: Selva Nair 
> >>
> >> This adds a minimal secure_memzero()
> >>
> >> Signed-off-by: Selva Nair 
> >
> > Feature-ACK, Code-NAK, but just because David is planning to export
> > secure_memzero() to plugins from OpenVPN proper - mentioned just today
> > on IRC
> >
> > 17:09 <@dazo> syzzer: just thinking aloud (remembered our discussion on
> wiping
> >   passwords securely in plug-ins) ... what do you think about
> >   exposing secure_memzero() to plug-ins, similar to what we
> do with
> >   plugin_{log,vlog}()?
> > 17:09 <@dazo> to avoid each plug-in needing to re-implement this
> > 17:09 <@syzzer> yeah, sounds useful
> > 17:10 <@dazo> I'll send a patch doing that too (should be quick to solve)
> >
> >
> > So I'd postpone this until David's patch plus instructions for plugin
> > authors show up.
> >
> > Good timing :-)
>
> Indeed :)
>

Good to know.. This happened to be a dependency for the second patch I sent
(parsing static challenge in auth-pam).


>
> So the patch exporting secure_memzero() is on the way to the mailing
> list now.
>
> To use this, you need to switch the openvpn_plugin_open_v1() to
> openvpn_plugin_open_v3().  The API on this function is quite different,
> but you shouldn't need to tweak too much.  All the pointers are
> available in the new struct pointers the _v3 function uses.
>
> Then you basically can declare a global variable like this:
>
>   plugin_secure_memzero_t ovpn_secure_memzero = NULL;


OK, I'll change this to use v3.

For the static-challenge patch I call openvpn_base64_decode() by compiling
base64.c into the plugin. Instead, can we also get base64_encode/decode
available as callbacks?

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] Set a low interface metric for tap adapter when block-outside-dns is in use

[Selva Nair]

Hi,

On Thu, May 4, 2017 at 1:36 PM, ValdikSS  wrote:

>
> Windows 10 before Creators Update used to resolve DNS using all available
> adapters and IP addresses in parallel.
> Now it still resolves addresses using all available adapters but in a
> round-robin way, beginning with random adapter.
>

Hmm... If it starts with a random adapter, this metric lowering is not the
right fix, isn't it? Or did you mean to say  "starting from lowest metric
adapter"?


> This behaviour introduces significant delay when block-outside-dns is in
> use.
> Fortunately, setting low metric for the TAP interface solves this issue,
> making Windows always pick with TAP adapter first.


Well, I'm looking for to excuses to avoid the hard work of a careful review
:)  If reducing metric is indeed the right approach in this cat and mouse
game with MS, I'll try to look through this over the weekend.

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] plugin: Export base64 encode and decode functions

[Selva Nair]

Hi,

Thanks for the patch exporting base64_encode/decode

A quick question/comment though: quoting from your sample base64.c

On Fri, May 5, 2017 at 5:46 PM, David Sommerseth  wrote:

> +/*  Which callbacks to intercept.  */
> +ret->type_mask =
> +OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_TLS_VERIFY)
> +|OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_CONNECT_V2);
> +
> +/* we don't need a plug-in context in this example, but OpenVPN
> expects "something" */
> +ret->handle = calloc(1, 1);
>

While this looks right[*], in openvpn-plugin.h (lines 344-370) the handle
in the _return struct
is defined as openvpn_plugin_handle_t * where that type itself is void *,
making handle void **.
Also the comment accompanying  "struct openvpn_plugin_args_open_return"
indicates
*handle should be set to the pointer to the context but that cant be right.

The compiler will not warn about void * and void ** mismatches, so ignoring
the definition and
saving the context pointer in ret->handle works,  but the declaration of
handle in the return struct
should be simply openvpn_plugin_handle_t without the *. The same comment
also refers to
*type_mask instead of type_mask.which is not right either.
.
Am I missing something?

Selva

[*] By the way, this being an example, it may be best to show the correct
type by casting the
return value of calloc to (openvpn_plugin_handle_t)
--
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] plugin: Export base64 encode and decode functions

[Selva Nair]

Hi,

Thanks for the follow up with details. I also put some further thought into
this overnight and totally agree with your observations. With one exception
:)

On Mon, May 8, 2017 at 9:56 AM, David Sommerseth  wrote:

> I think it is a bit too risky to actually fix the plug-in API to fix
> this.  So we need to use (openvpn_plugin_handle_t *) in the v3 API.
>

We can fix it by removing the * without breaking the API. And I think that
is the right thing to do. In any case all working plugins that use v3 has
to be using handle (an not *handle) to store the context pointer and then
retrieving it as passed back in the func call.

A code that sets *handle = context_ptr is clearly wrong and would segfault
Any that sets handle = &context_ptr would get the wrong pointer back in
the func call so would not function properly.

So both those usages can be assumed not to exist.

So the only concern would be plugins which set handle =
(openvpn_plugin_handle_t *) context_ptr. That is only a cast issue and will
continue to compile and "work" even if the struct is changed to have
"openvpn_plugin_handle_t handle).


> I'll update the patch accordingly.
>

So I suggest to change the API to define handle in the return struct as
openvpn_plugin_handle_t without the *.  It also avoids giving the wrong
impression that one should set handle = &context_ptr in the _open_v3() call.

> [*] By the way, this being an example, it may be best to show the
> > correct type by casting the
> > return value of calloc to (openvpn_plugin_handle_t)
>
> Yes, but shouldn't it be `(openvpn_plugin_handle_t *)` ?
>

I assumed the fix would be to change the struct.

Thanks,

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] plugin: Fix documentation typo for type_mask

[Selva Nair]

Hi,

On Mon, May 8, 2017 at 10:19 AM, David Sommerseth 
wrote:

>   *
>   * STRUCT MEMBERS
>   *
> - * *type_mask : The plug-in should set this value to the logical OR of
> all script
> + * type_mask  : The plug-in should set this value to the logical OR of
> all script
>   *  types which the plug-in wants to intercept.  For example,
> if the
>   *  script wants to intercept the client-connect and
> client-disconnect
>   *  script types:
>   *
> - *  *type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PL
> UGIN_CLIENT_CONNECT)
> + *  type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PL
> UGIN_CLIENT_CONNECT)
>   * | OPENVPN_PLUGIN_MASK(OPENVPN_PL
> UGIN_CLIENT_DISCONNECT)
>   *
>   * *handle :Pointer to a global plug-in context, created by the
> plug-in.  This pointer
>


Asking to set *handle to context_ptr is wrong too. One can't dereference
handle. So it will have to be interpreted as set handle to pointer to
pointer to the context but that is wrong.

But as discussed in the other thread I think we should also change the
definition of handle in this struct.

Reference to *handle in line 390 needs correction too (comment only --
struct is fine).

Thanks,

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] Set a low interface metric for tap adapter when block-outside-dns is in use

[Selva Nair]

Hi,

Please bear with me for making a few more comments. This close to final so
only
a few minor issues.

On Thu, May 4, 2017 at 1:36 PM, ValdikSS  wrote:
>
> Windows 10 before Creators Update used to resolve DNS using all available
> adapters and IP addresses in parallel.
> Now it still resolves addresses using all available adapters but in a
> round-robin way, beginning with random adapter.
> This behaviour introduces significant delay when block-outside-dns is in
> use.
> Fortunately, setting low metric for the TAP interface solves this issue,
> making Windows always pick with TAP adapter first.
>

Nitpicking here, but anyway a new version is required (see below), so
it would be nice if you wrap lines in commit message to something
like about 70 to 80 at most. Not sure whether we have a policy on this..

diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> index e31765e..9d49355 100644
> --- a/src/openvpn/block_dns.c
> +++ b/src/openvpn/block_dns.c
> @@ -341,4 +341,79 @@ delete_block_dns_filters(HANDLE engine_handle)
>  return err;
>  }
> +/*
> + * Returns interface metric value for specified interface index.
> + *
> + * Arguments:
> + *   index : The index of TAP adapter.
> + *   family: Address family (AF_INET for IPv4 and AF_INET6 for
> IPv6).
> + * Returns positive metric value or zero for automatic metric on success,
> + * a less then zero error code on failure.
> + */
> +
> +int
> +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
> +{
> +DWORD err = 0;
> +MIB_IPINTERFACE_ROW ipiface;
> +InitializeIpInterfaceEntry(&ipiface);


InitializeIpInterfaceEntry() is missing in all but very recent mingw32
versions
(their commit logs show it was added in early 2015) so we may need
to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1
and its
not there. I believe Samuli's build system uses an even older version.

Note that 64 bit build still succeeds with a warning but 32 bit will fail
because
of stdcall. Yes, its stdcall though MSDN doesn't mention it as such.


>  typedef void (*block_dns_msg_handler_t) (DWORD err, const char *msg);
>
>  DWORD
> @@ -36,5 +39,32 @@ DWORD
>  add_block_dns_filters(HANDLE *engine, int iface_index, const WCHAR
> *exe_path,
>block_dns_msg_handler_t msg_handler_callback);
>
> +/**
> + * Sets interface metric value for specified interface index
>

This would be "Get" or "Return" not "Set"


> + *
> + * @param index The index of TAP adapter
> + * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
> + *
> + * @return positive metric value or zero for automatic metric on success,
> + * a less then zero error code on failure.
> + */
> +
> +int
> +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY
> family);
>
>
..



> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index 18e7aee..dc411fc 100644
> --- a/src/openvpn/win32.c
> +++ b/src/openvpn/win32.c
> @@ -61,6 +61,11 @@
>  static HANDLE m_hEngineHandle = NULL; /* GLOBAL */
>
>  /*
> + * TAP adapter original metric value
> + */
> +static int tap_metric = -1; /* GLOBAL */
> +
> +/*
>   * Windows internal socket API state (opaque).
>   */
>  static struct WSAData wsa_state; /* GLOBAL */
> @@ -1337,6 +1342,21 @@ win_wfp_block_dns(const NET_IFINDEX index, const
> HANDLE msg_channel)
>
>  status = add_block_dns_filters(&m_hEngineHandle, index, openvpnpath,
> block_dns_msg_handler);
> +if (status == 0)
> +{
> +tap_metric = get_interface_metric(index, AF_INET);
>

The current metric is queried and saved only for v4 while the metric is
set and restored for both v4 and v6. Isn't it necessary to also save the
metric for both independently? I'm not sure whether anyone really uses
different metric for v4 and v6 but Windows allows it.


> +if (tap_metric < 0)
> +{
> +/* error, should not restore metric */
> +tap_metric = -1;
> +}
> +status = set_interface_metric(index, AF_INET,
> BLOCK_DNS_IFACE_METRIC);
> +if (!status)
> +{
> +set_interface_metric(index, AF_INET6, BLOCK_DNS_IFACE_METRIC);
> +}
> +}
> +
>  ret = (status == 0);
>
>  out:
>
>
> @@ -1325,6 +1368,7 @@ static VOID
>  Undo(undo_lists_t *lists)
>  {
>  undo_type_t type;
> +block_dns_data_t *interface_data;
>  for (type = 0; type < _undo_type_max; type++)
>  {
>  list_item_t **pnext = &(*lists)[type];
> @@ -1350,8 +1394,15 @@ Undo(undo_lists_t *lists)
>  break;
>
>  case block_dns:
> -delete_block_dns_filters(item->data);
> -item->data = NULL;
> +interface_data = (block_dns_data_t*)(item->data);
> +delete_block_dns_filters(interface_data->engine);
> +if (interface_data->metric > 0)
>

Here ">" should be ">=0", else automati

Re: [Openvpn-devel] [PATCH] plugin: Fix documentation typo for type_mask

[Selva Nair]

Hi,

On Mon, May 8, 2017 at 10:57 AM, David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 08/05/17 16:38, Selva Nair wrote:
> > Hi,
> >
>
On Mon, May 8, 2017 at 10:19 AM, David Sommerseth 
>>  wrote:
>>   *
>>   * STRUCT MEMBERS
>>   *
>> - * *type_mask : The plug-in should set this value to the logical OR of
>> all script
>> + * type_mask  : The plug-in should set this value to the logical OR of
>> all script
>>   *  types which the plug-in wants to intercept.  For
>> example, if the
>>   *  script wants to intercept the client-connect and
>> client-disconnect
>>   *  script types:
>>   *
>> - *  *type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PL
>> UGIN_CLIENT_CONNECT)
>> + *  type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PL
>> UGIN_CLIENT_CONNECT)
>>   * | OPENVPN_PLUGIN_MASK(OPENVPN_PL
>> UGIN_CLIENT_DISCONNECT)
>>   *
>>   * *handle :Pointer to a global plug-in context, created by the
>> plug-in.  This pointer
>
>

>
> >
> > Asking to set *handle to context_ptr is wrong too. One can't
> > dereference handle. So it will have to be interpreted as set handle
> > to pointer to pointer to the context but that is wrong.
> >
> > But as discussed in the other thread I think we should also change
> > the definition of handle in this struct.
> >
> > Reference to *handle in line 390 needs correction too (comment only
> > -- struct is fine).
>
> Yes, I saw that, but thought we should fix that when we know what is
> the right solution for the *handle vs handle.   Just to keep things
> separate.


OK, that makes sense.  Then this is good to go.

ACK.

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] plugin: Export secure_memzero() to plug-ins

[Selva Nair]

Hi,

On Fri, May 5, 2017 at 2:46 PM, David Sommerseth  wrote:

> The provides plug-ins with a safe and secure way to santize sensitive
> information such as passwords, by re-using the secure_memzero()
> implementation in OpenVPN.
>
> Signed-off-by: David Sommerseth 
> ---
>  include/openvpn-plugin.h.in | 25 ++---
>  src/openvpn/plugin.c|  3 ++-
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
> index 0b303520..ec83f3a6 100644
> --- a/include/openvpn-plugin.h.in
> +++ b/include/openvpn-plugin.h.in
> @@ -199,7 +199,8 @@ struct openvpn_plugin_string_list
>
>  /* openvpn_plugin_{open,func}_v3() related structs */
>
> -/* Defines version of the v3 plugin argument structs
> +/**
> + * Defines version of the v3 plugin argument structs
>   *
>   * Whenever one or more of these structs are modified, this constant
>   * must be updated.  A changelog should be appended in this comment
> @@ -218,8 +219,10 @@ struct openvpn_plugin_string_list
>   *3  Added ovpn_version, ovpn_version_major, ovpn_version_minor
>   *   and ovpn_version_patch to provide the runtime version of
>   *   OpenVPN to plug-ins.
> + *
> + *4  Exported secure_memzero() as plugin_secure_memzero()
>   */
> -#define OPENVPN_PLUGINv3_STRUCTVER 3
> +#define OPENVPN_PLUGINv3_STRUCTVER 4
>
>  /**
>   * Definitions needed for the plug-in callback functions.
> @@ -255,10 +258,19 @@ typedef void (*plugin_vlog_t)(openvpn_plugin_log_flags_t
> flags,
>const char *plugin_name,
>const char *format,
>va_list arglist) _ovpn_chk_fmt (3, 0);
> -
>  #undef _ovpn_chk_fmt
>
>  /**
> + *  Export of secure_memzero() to be used inside plug-ins
> + *
> + *  @param data   Pointer to data to zeroise
> + *  @param lenLength of data, in bytes
> + *
> + */
> +typedef void (*plugin_secure_memzero_t)(void *data, size_t len);
> +
> +
> +/**
>   * Used by the openvpn_plugin_open_v3() function to pass callback
>   * function pointers to the plug-in.
>   *
> @@ -267,11 +279,18 @@ typedef void (*plugin_vlog_t)(openvpn_plugin_log_flags_t
> flags,
>   *   Messages will only be displayed if the plugin_name
> parameter
>   *   is set. PLOG_DEBUG messages will only be displayed with
> plug-in
>   *   debug log verbosity (at the time of writing that's verb
> >= 7).
> + *
> + * plugin_secure_memzero
> + * : Use this function to securely wipe sensitive information
> from
> + *   memory.  This function is declared in a way that the
> compiler
> + *   will not remove these function calls during the compiler
> + *   optimization phase.
>   */
>  struct openvpn_plugin_callbacks
>  {
>  plugin_log_t plugin_log;
>  plugin_vlog_t plugin_vlog;
> +plugin_secure_memzero_t plugin_secure_memzero;
>  };
>
>  /**
> diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
> index 05cbae3e..a652d528 100644
> --- a/src/openvpn/plugin.c
> +++ b/src/openvpn/plugin.c
> @@ -410,7 +410,8 @@ plugin_log(openvpn_plugin_log_flags_t flags, const
> char *name, const char *forma
>
>  static struct openvpn_plugin_callbacks callbacks = {
>  plugin_log,
> -plugin_vlog
> +plugin_vlog,
> +secure_memzero   /* plugin_secure_memzero */
>  };


This avoids code duplication in the plugin and works as expected.

ACK.

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

[Openvpn-devel] [PATCH 1/2 v2] In auth-pam plugin clear the password after use

[selva . nair]

From: Selva Nair 

v2: Change the plugin open to use v3 API so that
  openvpn_secure_memzero() exported from OpenVPN can be used.

Note: context is cast as (openvpn_plugin_handle_t *) for consistency
with the current plugin header. If/when the header is fixed, change
this cast as well.

Signed-off-by: Selva Nair 
---
 src/plugins/auth-pam/auth-pam.c   | 31 ++-
 src/plugins/auth-pam/auth-pam.exports |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..f3566c7 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -63,6 +63,9 @@
 #define RESPONSE_VERIFY_SUCCEEDED 12
 #define RESPONSE_VERIFY_FAILED13
 
+/* Pointers to functions exported from openvpn */
+static plugin_secure_memzero_t plugin_secure_memzero = NULL;
+
 /*
  * Plugin state, used by foreground
  */
@@ -274,8 +277,10 @@ name_value_match(const char *query, const char *match)
 return strncasecmp(match, query, strlen(match)) == 0;
 }
 
-OPENVPN_EXPORT openvpn_plugin_handle_t
-openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char 
*envp[])
+OPENVPN_EXPORT int
+openvpn_plugin_open_v3(const int v3structver,
+   struct openvpn_plugin_args_open_in const *args,
+   struct openvpn_plugin_args_open_return *ret)
 {
 pid_t pid;
 int fd[2];
@@ -285,6 +290,16 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 
 const int base_parms = 2;
 
+const char **argv = args->argv;
+const char **envp = args->envp;
+
+/* Check API compatibility */
+if (v3structver != OPENVPN_PLUGINv3_STRUCTVER)
+{
+fprintf(stderr, "AUTH-PAM: This plugin is incompatible with the 
running version of OpenVPN\n");
+return OPENVPN_PLUGIN_FUNC_ERROR;
+}
+
 /*
  * Allocate our context
  */
@@ -298,7 +313,10 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 /*
  * Intercept the --auth-user-pass-verify callback.
  */
-*type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+ret->type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+
+/* Save global pointers to functions exported from openvpn */
+plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
 
 /*
  * Make sure we have two string arguments: the first is the .so name,
@@ -386,7 +404,8 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 if (status == RESPONSE_INIT_SUCCEEDED)
 {
 context->foreground_fd = fd[0];
-return (openvpn_plugin_handle_t) context;
+ret->handle = (openvpn_plugin_handle_t *) context;
+return OPENVPN_PLUGIN_FUNC_SUCCESS;
 }
 }
 else
@@ -420,7 +439,7 @@ error:
 {
 free(context);
 }
-return NULL;
+return OPENVPN_PLUGIN_FUNC_ERROR;
 }
 
 OPENVPN_EXPORT int
@@ -785,6 +804,7 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 goto done;
 }
 }
+plugin_secure_memzero(up.password, sizeof(up.password));
 break;
 
 case COMMAND_EXIT:
@@ -802,6 +822,7 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 }
 done:
 
+plugin_secure_memzero(up.password, sizeof(up.password));
 #ifdef USE_PAM_DLOPEN
 dlclose_pam();
 #endif
diff --git a/src/plugins/auth-pam/auth-pam.exports 
b/src/plugins/auth-pam/auth-pam.exports
index b07937c..597e33f 100644
--- a/src/plugins/auth-pam/auth-pam.exports
+++ b/src/plugins/auth-pam/auth-pam.exports
@@ -1,4 +1,4 @@
-openvpn_plugin_open_v1
+openvpn_plugin_open_v3
 openvpn_plugin_func_v1
 openvpn_plugin_close_v1
 openvpn_plugin_abort_v1
-- 
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 1/2 v2] In auth-pam plugin clear the password after use

[Selva Nair]

Hi,

On Tue, May 9, 2017 at 1:34 PM, David Sommerseth  wrote:

> If the plug-in built and packaged separately and that build is not tied
> to OpenVPN itself, this can make this plug-in fail without any
> particular real reason if the OpenVPN binary gets updated independently.
>
> Even though not explicitly documented, we should never remove any
> information from the plug-in v3 structs, only append to and only append
> at the end of the existing structs.
>

I just assumed these "official" plugins will always be built and
distributed with openvpn core.
But if future revisions is guaranteed not to reorder/remove items in the
structs, a less strict check
is definitely better.


> The plug-ins in src/plugins should use, which ensures forward
> compatibility against the OpenVPN binary:
>
>if (v3structver < OPENVPN_PLUGINv3_STRUCTVER)
>

Yes, will change to this.

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] Always clear username/password from memory on error

[Selva Nair]

Hi,

On Tue, May 9, 2017 at 1:47 PM, David Sommerseth  wrote:

> That said, I think we should fix secure_memzero() to just return if the
> input pointer is NULL.  And even though most compilers do initialize
> variables, I think it's good to be defensive here and initialize `up` too.
>

No, compiler will not initialize such non-static local variables. In fact
in this case gcc will warn (with -Wall) that "up" may be used uninitalized
because of code paths where "up" is not allocated as you pointed out.

In that sense its better not to initialize such variables as initialization
can hide errors.

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

[Openvpn-devel] [PATCH 1/2 v3] In auth-pam plugin clear the password after use

[selva . nair]

From: Selva Nair 

v2: Change the plugin open to use v3 API so that secure_memzero()
exported from OpenVPN can be used.
v3: Relaxe API compatibility check: struct version 4 or higher
will have secure_memzero exported.

Note: context is cast as (openvpn_plugin_handle_t *) for consistency
with the current plugin header. If/when the header is fixed, change
this cast as well.

Signed-off-by: Selva Nair 
---
 src/plugins/auth-pam/auth-pam.c   | 31 ++-
 src/plugins/auth-pam/auth-pam.exports |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..54471a3 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -63,6 +63,9 @@
 #define RESPONSE_VERIFY_SUCCEEDED 12
 #define RESPONSE_VERIFY_FAILED13
 
+/* Pointers to functions exported from openvpn */
+static plugin_secure_memzero_t plugin_secure_memzero = NULL;
+
 /*
  * Plugin state, used by foreground
  */
@@ -274,8 +277,10 @@ name_value_match(const char *query, const char *match)
 return strncasecmp(match, query, strlen(match)) == 0;
 }
 
-OPENVPN_EXPORT openvpn_plugin_handle_t
-openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char 
*envp[])
+OPENVPN_EXPORT int
+openvpn_plugin_open_v3(const int v3structver,
+   struct openvpn_plugin_args_open_in const *args,
+   struct openvpn_plugin_args_open_return *ret)
 {
 pid_t pid;
 int fd[2];
@@ -285,6 +290,16 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 
 const int base_parms = 2;
 
+const char **argv = args->argv;
+const char **envp = args->envp;
+
+/* Check API compatibility -- struct version 4 or higher needed */
+if (v3structver < 4)
+{
+fprintf(stderr, "AUTH-PAM: This plugin is incompatible with the 
running version of OpenVPN\n");
+return OPENVPN_PLUGIN_FUNC_ERROR;
+}
+
 /*
  * Allocate our context
  */
@@ -298,7 +313,10 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 /*
  * Intercept the --auth-user-pass-verify callback.
  */
-*type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+ret->type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+
+/* Save global pointers to functions exported from openvpn */
+plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
 
 /*
  * Make sure we have two string arguments: the first is the .so name,
@@ -386,7 +404,8 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char 
*argv[], const char *
 if (status == RESPONSE_INIT_SUCCEEDED)
 {
 context->foreground_fd = fd[0];
-return (openvpn_plugin_handle_t) context;
+ret->handle = (openvpn_plugin_handle_t *) context;
+return OPENVPN_PLUGIN_FUNC_SUCCESS;
 }
 }
 else
@@ -420,7 +439,7 @@ error:
 {
 free(context);
 }
-return NULL;
+return OPENVPN_PLUGIN_FUNC_ERROR;
 }
 
 OPENVPN_EXPORT int
@@ -785,6 +804,7 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 goto done;
 }
 }
+plugin_secure_memzero(up.password, sizeof(up.password));
 break;
 
 case COMMAND_EXIT:
@@ -802,6 +822,7 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 }
 done:
 
+plugin_secure_memzero(up.password, sizeof(up.password));
 #ifdef USE_PAM_DLOPEN
 dlclose_pam();
 #endif
diff --git a/src/plugins/auth-pam/auth-pam.exports 
b/src/plugins/auth-pam/auth-pam.exports
index b07937c..597e33f 100644
--- a/src/plugins/auth-pam/auth-pam.exports
+++ b/src/plugins/auth-pam/auth-pam.exports
@@ -1,4 +1,4 @@
-openvpn_plugin_open_v1
+openvpn_plugin_open_v3
 openvpn_plugin_func_v1
 openvpn_plugin_close_v1
 openvpn_plugin_abort_v1
-- 
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 v2] Document tls-crypt security considerations in man page

[Selva Nair]

Hi,

Looks good except for some typos:

On Tue, May 9, 2017 at 2:42 PM, Steffan Karger 
wrote:

> The tls-crypt commit message contained an elaborate discussion on the
> function's security properties.  This commit adds the gist of that
> discussion, "rotate keys periodically" to the man page.
>
> (The 'real' solution will follow later: add support for per-client
> tls-crypt keys.  That will make tls-crypt useful for VPN providers too.)
>
> Note to non-crypto-geek reviewers: please verify that this text is clear
> enough to explain you when you need to replace tls-crypt keys.
>
> Note to crypto-geek reviewers: please check the numbers - see the
> --tls-crypt commit message (c6e24fa3) for details.
>
> Signed-off-by: Steffan Karger 
> ---
> v2: clarify that 2^16 packets is a conservative estimate, use plural
> year*s*
>
>  doc/openvpn.8 | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index c3248fd..06f8c66 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5090,6 +5090,29 @@ In contrast to
>  .B \-\-tls\-crypt
>  does *not* require the user to set
>  .B \-\-key\-direction\fR.
> +
> +.B Security Considerations
> +
> +All peers use the same
> +.B \-\-tls-crypt
> +pre-shared group key to authenticate and encrypt control channel
> messages.  To
> +ensure that IV collisions remain unlikely, this key should not be used to
> +encrypt more than 2^48 client-to-server or 2^48 server-to-client control
>
+channel messages.  A typical initial negotiation is about 10 packets in
> each
> +direction.  Assuming both initial negotation and renogatiations are at
> most
>

"negotiation", "renegotiations"


> +2^16 (65536) packets (too be conservative), and (re)negotiations happen
> each
>

"to" instead of "too"


> +minute for each user (24/7), this limits the tls\-crypt key lifetime to
> 8171
> +years divided by the number of users.  So a setup with 1000 users should
> rotate
> +the key at least once each eight years.  (And a setup with 8000 users each
> +year.)
> +
> +If IV collisions were to occur, this could result in the security of
> +.B \-\-tls\-crypt
> +degrading to the same security as using
> +.B \-\-tls\-auth\fR.
> +That is, the control channel still benefits from the extra protection
> against
> +active man-in-the-middle-attacks and DoS attacks, but may no longer offer
> +extra privacy and post-quantum security on top of what TLS itself offers.
>  .\"*
>  .TP
>  .B \-\-askpass [file]
>

Reads well otherwise. So ACK assuming typos will be fixed :)

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] Set a low interface metric for tap adapter when block-outside-dns is in use

[Selva Nair]

On Wed, May 10, 2017 at 12:08 PM, ValdikSS  wrote:

>
> InitializeIpInterfaceEntry() is missing in all but very recent mingw32
> versions
> (their commit logs show it was added in early 2015) so we may need
> to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1
> and its
> not there. I believe Samuli's build system uses an even older version.
>
>
> I don't know how to do that correctly. Can you please help me?



In block-dns.c where "WFP-related defines and GUIDs not in mingw32" are
defined, add this

VOID NETIOAPI_API_
InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row );

Do a test build for 32 bit to see now this function compiles without
warnings
and links without errors. 32 bit is a more stringent test because only in
this
case the linker will fail to find stdcall functions without proper
declarations.

That should be it.

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 v2] Set a low interface metric for tap adapter when block-outside-dns is in use

[Selva Nair]

On Wed, May 10, 2017 at 2:47 PM, ValdikSS  wrote:

>
> Windows 10 before Creators Update used to resolve DNS using all
> available adapters and IP addresses in parallel. Now it still
> resolves addresses using all available adapters but in a round-robin
> way, beginning with random adapter.
> This behaviour introduces significant delay when block-outside-dns is
> in use. Fortunately, setting low metric for the TAP interface solves
> this issue, making Windows always pick TAP adapter first and disable
> round-robin.
> ---
>  src/openvpn/block_dns.c   | 78 ++
> +
>  src/openvpn/block_dns.h   | 30 +
>  src/openvpn/init.c|  4 +--
>  src/openvpn/win32.c   | 39 --
>  src/openvpn/win32.h   |  2 +-
>  src/openvpnserv/interactive.c | 70 +++---
>  6 files changed, 213 insertions(+), 10 deletions(-)



Looks good now.  ACK from me.

Did a quick test on Win7 -- metric changes to 3 and gets restored
independently for v4 and v6. No idea whether this resolves the Win 10 issue,
but should not have any adverse effects except for the metric change
which will also affect the metric of routes added.

Cross compiles without issues for 32 bit and 64 bit -- tested on Debain 8.7
with
mingw32-w64 included in the distribution.

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

[Openvpn-devel] [PATCH] Pass correct buffer size to GetModuleFileNameW()

[selva . nair]

From: Selva Nair 

Fixes finding 5.6 of OSTIF/Quarkslab audit

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

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 0cbf5fd..9a03681 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1334,8 +1334,8 @@ win_wfp_block_dns(const NET_IFINDEX index, const HANDLE 
msg_channel)
 goto out;
 }
 
-status = GetModuleFileNameW(NULL, openvpnpath, sizeof(openvpnpath));
-if (status == 0 || status == sizeof(openvpnpath))
+status = GetModuleFileNameW(NULL, openvpnpath, _countof(openvpnpath));
+if (status == 0 || status == _countof(openvpnpath))
 {
 msg(M_WARN|M_ERRNO, "block_dns: cannot get executable path");
 goto out;
-- 
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 applied] Re: Pass correct buffer size to GetModuleFileNameW()

[Selva Nair]

On Sat, May 13, 2017 at 2:17 PM, Gert Doering  wrote:

> ACK, thanks.  (No tests run whatsoever, but we've had a discussion about
> that on the security@ lists, and there was agreement that _countof is
> the thing to use - just nobody did it before, so thanks again :-) ).
>
> Your patch has been applied to the master and release/2.4 branch.
>
> Release/2.3 has "MAX_PATH" instead of sizeof or _countof, which is
> actually correct (but in contrast, it has no error checking for
> GetModuleFileNameW()...)
>

Oh, I missed that. Note to self: we should add error checking there. If the
buffer is too small we will get a truncated but null terminated name, so
that's not too bad. But if the call fails the buffer may contain random
bytes.

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

[Openvpn-devel] [PATCH for 2.3] Check for errors in the return value of GetModuleFileNameW()

[selva . nair]

From: Selva Nair 

Also replace MAX_PATH by _countof(openvpnpath) as the latter
is arguably more robust.

Signed-off-by: Selva Nair 
---
 src/openvpn/win32.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index b271597..56c3a1d 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1271,7 +1271,12 @@ win_wfp_block_dns (const NET_IFINDEX index)
 dmsg (D_LOW, "Tap Luid: %I64d", tapluid.Value);
 
 /* Get OpenVPN path. */
-GetModuleFileNameW(NULL, openvpnpath, MAX_PATH);
+status = GetModuleFileNameW(NULL, openvpnpath, _countof(openvpnpath));
+if (status == 0 || status == _countof(openvpnpath))
+{
+msg(M_WARN|M_ERRNO, "block_dns: failed to get executable path");
+goto err;
+}
 
 if (FwpmGetAppIdFromFileName0(openvpnpath, &openvpnblob) != ERROR_SUCCESS)
 goto err;
-- 
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] Correct the declaration of handle in 'struct openvpn_plugin_args_open_return'

[selva . nair]

From: Selva Nair 

- This is an opaque pointer so the change should not affect existing plugins.
  But it makes the code consistent and clears up the documentation as the handle
  pointer is treated as of type "openvpn_plugin_handle_t" in the rest of the 
code.

Signed-off-by: Selva Nair 
---
 include/openvpn-plugin.h.in | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index 5e6f874..b745307 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -355,7 +355,7 @@ struct openvpn_plugin_args_open_in
  *  type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_CONNECT)
  * | 
OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_DISCONNECT)
  *
- * *handle :Pointer to a global plug-in context, created by the plug-in.  
This pointer
+ * handle : Pointer to a global plug-in context, created by the plug-in.  
This pointer
  *  is passed on to the other plug-in calls.
  *
  * return_list : used to return data back to OpenVPN.
@@ -364,7 +364,7 @@ struct openvpn_plugin_args_open_in
 struct openvpn_plugin_args_open_return
 {
 int type_mask;
-openvpn_plugin_handle_t *handle;
+openvpn_plugin_handle_t handle;
 struct openvpn_plugin_string_list **return_list;
 };
 
@@ -386,9 +386,9 @@ struct openvpn_plugin_args_open_return
  *these variables are not actually written to the "official"
  *environmental variable store of the process.
  *
- * *handle : Pointer to a global plug-in context, created by the plug-in's 
openvpn_plugin_open_v3().
+ * handle : Pointer to a global plug-in context, created by the plug-in's 
openvpn_plugin_open_v3().
  *
- * *per_client_context : the per-client context pointer which was returned by
+ * per_client_context : the per-client context pointer which was returned by
  *openvpn_plugin_client_constructor_v1, if defined.
  *
  * current_cert_depth : Certificate depth of the certificate being passed over 
(only if compiled with ENABLE_CRYPTO defined)
-- 
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] block-outside-dns and persist-tun

[Selva Nair]

Hi,

Copying the -devel list:

On Sun, May 28, 2017 at 10:16 AM, ValdikSS  wrote:

> Pavel, a friend of mine, made a service to circumvent Ukrainian blocks of
> Russian websites. He configured OpenVPN TCP without persist-tun on the
> client side and pushes block-outside-dns from server.
>
> When he restarts OpenVPN server, DNS no longer works on the clients.
> Neither with or without VPN. Users say this can be fixed only with
> rebooting, I believe restarting service would help too.
>

Is this only with 2.4.2 or is 2.4.1 also affected?  As you imply, the
filters won't persist after the process ends (in this case the service),
restarting service should be enough to clear them. Further, even if the
openvpn client process terminates without removing the filters, the service
should clean up all filters added in that session during the undo()
processing. However, that wont happen if the openvpn.exe process fails to
exit. Verify that a stale client process is not hanging around.


>
> I tried to do exactly what he did with Windows 7 and OpenVPN 2.4.2 and I
> can't reproduce this bug. I think service in some cases loses TAP adapter
> index before unblocking DNS.
>

The tap adapter index is used to allow dns traffic through it, not block
it, so I would think the failure is in unblocking dns through non-tap
adapters. If that is the case, dns should start working again through the
tunnel when the client reconnects.

Anyway, we need to see the client logs and any error event logged by the
service when this happens. Can you get the user to open a ticket with logs?


>
> Works fine with persist-tun on client side.
>

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] [OpenVPN/openvpn-gui] better handling of interactive service failure (#168)

[Selva Nair]

Hi,

Copying openvpn-devel:
As this is related to openvpn best to have this discussion in the devel
list, I suppose.
(see also:
https://github.com/OpenVPN/openvpn-gui/issues/168#issuecomment-305250704)

On Wed, May 31, 2017 at 12:58 PM, Gert Doering 
wrote:

> On Wed, May 31, 2017 at 09:43:21AM -0700, Selva Nair wrote:
> > As I said, get openvpn to report route errors in the status and then we
> can
> > add a warning to the status popup, turn the icon red etc instead of the
> > current misleading "successfully connected" behaviour.
>
> This is actually a discussion I was trying to have a long time ago
> (a few years) - "why do we ignore route addition errors?".
>
> The IPv6 code doesn't (because I think that errors are errors, not
> warnings...) and that was always some sort of weird asymmetry...
>
> I still don't know the reasoning here, but I suspect it's something along
> "you push a route that is identical to the local subnet" (192.168.1.0/24,
> for example, because the user happens to be in a bad NAT network) and
> "all of a sudden it fails"... so this might need more discussion, and
> also some code cleanups to gracefully handle situations where an error
> is "tolerated".
>

That and some route addition errors like "route already exists" are often
benign. So a fatal error is not appropriate. But, IIRC,
 openvpn_execve_check only allows printing of errors as FATAL or WARN.
Currently we do not parse the log message flags (error vs warning etc.) in
the Windows GUI, but that could be improved if openvpn can log route errors
like access denied as such.

In any case, the status reported to the management when connected with
errors should be something other than "CONNECTED,SUCCESS" -- say
"CONNECTED,ROUTE-FAILED" etc. so that  UI can intimate the user.

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] block-outside-dns and persist-tun

[Selva Nair]

On Sat, Jun 3, 2017 at 4:13 PM, ValdikSS  wrote:

>
> You can skip through comments on https://zaborona.help/ to see some
> screenshots and logs.
> Like this one: https://zaborona.help/faq.html#comment-3328754341


I did not find any related to failure to remove WFP filters. That specific
comment link reads


Sun May 28 18:07:25 2017 Block_DNS: WFP engine opened
Sun May 28 18:07:25 2017 Error in add_block_dns_filters(): add_sublayer:
failed to add persistent sublayer : Отказано в доступе. [status=0x5]
Sun May 28 18:07:25 2017 Blocking DNS failed!
Sun May 28 18:07:25 2017 Exiting due to fatal error


Obviously "access denied" due to not running as admin and service not in
use.

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 2/2] Parse static challenge response in auth-pam plugin

[Selva Nair]

On Mon, Jun 12, 2017 at 2:14 PM, Gert Doering  wrote:

> Hi,
>
> wading through my heap of mails that did not get proper attention...
>
> On Fri, May 05, 2017 at 02:24:02PM -0400, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > If static challenge is in use, the password passed to the plugin by
> openvpn
> > is of the form "SCRV1:base64-pass:base64-response". Parse this string to
> > separate it into password and response and use them to respond to queries
> > in the pam conversation function.
> >
> > On the plugin parameters line the substitution keyword for the static
> > challenge response is "OTP". For example, for pam config named "test"
> that
> > prompts for "user", "password" and "pin", use
> >
> > plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"
>
> What is the status of this one?  Does it need updating after 1/2 got
> changed to v2 and v3 are these independent enough that 2/2 is standalone?
>
> From a cursory glance, it calls secure_memzero() which is now, I think,
> plugin_secure_memzero() - right?
>
>
> I also seem to remember discussions between you and David regarding
> base64 function exporting - what's the state on this?
>


I have a version 2 that uses exported base64 but waiting on the following
patch to get an NAK or ACK to finalize it.

   https://www.mail-archive.com/openvpn-devel@lists.sourcefo
rge.net/msg14655.html

That would also help David's base64 export patch revised, reviewed and
merged. For background, see the thread

https://www.mail-archive.com/openvpn-devel@lists.
sourceforge.net/msg14577.html

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Wed, Jun 7, 2017 at 12:41 PM, debbie10t  wrote:

> Hi,
>
> I have a basic setup and discovered that my W10 client was assigning a
> second IPv6 address to TAP even though it is *not* being pushed by the
> server.  The second address is an old address from a server that I
> sometimes connect to.  The server is still running but the client is not
> connected to it.
>
> The most significant information here is that this *only* happens when
> using the openvpn-GUI + interactive-service.


On Mon, Jun 12, 2017 at 10:28 AM, debbie10t  wrote:

> Is there no explanation ?


The service should delete the address when openvpn terminates so can't
think of any reason why an old address would linger even if the same tap
adapter was used for the previous connection.

Does this happen only on Win 10?

Wed Jun 07 16:33:53 2017 us=125353 ROUTE: route addition failed using
> service: The object already exists.   [status=5010 if_index=17]


This is a known issue (route not deleted on termination so fails to
recreate later) and is not a critical. FWIW, there is a patch on the ML to
fix it.

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 1:25 PM, Илья Шипицин  wrote:

> I decided to try some tests .. in fact I only did one test.
> I rebooted the PC and now the second IP address has gone.
> (Windows Fast shutdown/reboot disabled .. so full reboot)
>
> I also tried to recreate the problem but so far cannot ..
>
>
> It might happen if openvpn dies without a chance of deleting associated
> ipv6 routes.
>

Not if openvpn dies: interactive service will do the cleanup in that case.
If the service dies, yes, the address will not be removed but even in that
case address added to one interface cannot show up on another.

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 2:01 PM, debbie10t  wrote:

> As client:
>Config-1 assigns 12fc:1918::10:36:101:110/112 to TAP (in tun mode)
>Config-2 assigns 12fc:1918::10:8:0:110/112 to TAP (in tun mode)
>
> Following:
>
> 1. Administrator command prompt - execute openvp config-1.ovpn
>TAP is assigned 12fc:1918::10:36:101:110/112
> 2. Terminate by closing [X] the command prompt
> 3. Using GUI + Interactive-service start config-2.ovpn
>TAP is assigned 12fc:1918::10:36:101:110/112
>  & TAP is assigned 12fc:1918::10:8:0:110/112
>
> Please try for yourself and post results.
>

I haven't tested this, but if both connections use the same adapter, this
looks possible as you kill the first process without giving it a chance to
remove the IP first. Addresses are added with store=active so would
disappear on reboot as you saw.

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 3:25 PM, Gert Doering  wrote:

> > I haven't tested this, but if both connections use the same adapter, this
> > looks possible as you kill the first process without giving it a chance
> to
> > remove the IP first. Addresses are added with store=active so would
> > disappear on reboot as you saw.
>
> Right.  If you a) run as admin, and b) force-kill openvpn (which "close
> the command window" might do), yes, there is no way to clean up.
>
> In that case, it's just plain PEBKAC :-)


Right :)  But we could probably do better using Set instead of Add while
the address is set using the service. I'm not that familiar with ipapi, but
looks like SetUnicastIpAddressEnrty instead of AddUnicastIpAddressEntry
may be the right thing.

The netsh command used when service is not in use does correctly do a set
not add.

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 3:54 PM, Arne Schwabe  wrote:

> >
> >
> > if user is administrator, interactive service is not used.
> > well, I did miss that about interactive service.
> >
>
> I wonder we should always use the interactive service if available and
> add (dont-use-interactive) option, so behaviour is always the same.


This was done for security -- some Windows versions have broken handling of
passing credentials through named pipe which could be used for privilege
escalation. I have seen this exploit work only on Windows XP[*], but to be
cautious we opted not to allow openvpn running as admin connect to the
service pipe.

But anyway, in this case its the service that's doing the wrong thing.

Selva

[*] On XP, a rogue program running as user can gain admin rights if a
program running as admin connects to it through a named pipe.
--
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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 3:37 PM, Gert Doering  wrote:

> On Tue, Jun 13, 2017 at 03:33:35PM -0400, Selva Nair wrote:
> > Right :)  But we could probably do better using Set instead of Add while
> > the address is set using the service. I'm not that familiar with ipapi,
> but
> > looks like SetUnicastIpAddressEnrty instead of AddUnicastIpAddressEntry
> > may be the right thing.
> >
> > The netsh command used when service is not in use does correctly do a set
> > not add.
>
> Good point.  Wouldn't solve the first problem, but at least a subsequent
> run would "magically fix it", provided the same tap interface is used
> (which, I think, is the case in the great majority of windows
> installations)


Okay, I can say that the issue is indeed the service effectively doing an
"add address" instead
of "set address". Reproduced this without using an admin run:

1. Connect to server 1 using GUI/iservice
2. Kill iservice using taskmanager or taskkill (do not do sc stop
service-name.. as that will do a graceful stop)
3. Start the service again
4. Disconnect  from server 1
At this point "netsh int ipv6 show addr" will show that v6 addres is
retained although the connection is down
4. connect to server 2 using GUI/iservice

Now the adapter will have two ipv6 addresses.

On Tue, Jun 13, 2017 at 3:50 PM, Samuli Seppänen  wrote:

> I encountered the issue with the Powershell test suite
> (openvpn-windows-test) which ran through a bunch of t_client test
> configs and forcibly killed the OpenVPN process at the end of each test.
> All ping6 tests except the first one would then fail, because adding a
> default IPv6 route failed as one existed already.


Probably the same issue. The real culprit here is force-killing without
impunity. I'll be posting a PR to the GUI that allows sending connect,
disconnect and exit commands to the running GUI, so we could use that in
the script. That said using "set address" in the service will mitigate this
to some extent -- looks easy to do a patch..

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Tue, Jun 13, 2017 at 4:30 PM, Илья Шипицин  wrote:

> 2017-06-14 1:05 GMT+05:00 Selva Nair :
>
>>
>> On Tue, Jun 13, 2017 at 3:54 PM, Arne Schwabe  wrote:
>>
>>> >
>>> >
>>> > if user is administrator, interactive service is not used.
>>> > well, I did miss that about interactive service.
>>> >
>>>
>>> I wonder we should always use the interactive service if available and
>>> add (dont-use-interactive) option, so behaviour is always the same.
>>
>>
>> This was done for security -- some Windows versions have broken handling
>> of passing credentials through named pipe which could be used for privilege
>> escalation. I have seen this exploit work only on Windows XP[*], but to be
>> cautious we opted not to allow openvpn running as admin connect to the
>> service pipe.
>>
>> But anyway, in this case its the service that's doing the wrong thing.
>>
>
> well, I'm lost here.
>
> sounds like "we do not use interactive service if user is already an
> administrator ... due to possible privilege escalation", right ? escalation
> to "system" ?
>

No, just escalation from user to admin. Think of a system where iservice is
not running. A user could start a rogue process in the background that
listens on the service pipe. This is easily done do as the service pipe
uses a fixed name and no authentication is needed to connect to it. Then an
admin who starts the GUI will connect to the pipe and let the rogue program
gain admin rights. It takes only a few line sof code to exploit this on XP
-- I have not been able to exploit this on Vista but not 100% sure it has
been fixed for good on Vista+.

For more details see, for example,
https://labs.portcullis.co.uk/blog/windows-named-pipes-there-and-back-again/

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] W10 Client assigns old AND new IPv6 address to TAP with GUI+Service but not with cmd prompt

[Selva Nair]

On Thu, Jun 15, 2017 at 8:32 AM, David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 13/06/17 22:51, Selva Nair wrote:
> > It takes only a few line sof code to exploit this on XP -- I have not
> > been able to exploit this on Vista but not 100% sure it has been
> > fixed for good on Vista+.
>
> But do we really care much for anything older than Win7 these days?  For
> v2.3, perhaps yes - but v2.4 is the one introducing the interactive
> service.


Even so, I think its still prudent to err on the side of caution and not
allow a client running as admin hand out impersonation rights to a server
it connects to, unless some authentication could be put in place.

Note: Here client is the GUI and server is anything that listens on the
service named pipe --- which is normally the service, but could be a rogue
program. I'm being paranoid, and impersonation via named pipe is probably
secure in recent versions of Windows.

If the GUI is running as admin, so will openvpn and it then doesn't need
the service for any tasks.  So no functionality is lost by not using the
service. However, running the GUI as admin is not a good idea, so a better
way would be to check whether iservice is available and if so drop
privileges and proceed. That way openvpn will also run with limited
privileges.

Indeed it would ease code maintenance if we can drop the numerous ways of
setting routes etc. and exclusively use the service. Until that becomes
acceptable, the use of iservice while admin brings in little benefit, if
any.

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 2/2] Parse static challenge response in auth-pam plugin

[Selva Nair]

On Mon, Jun 12, 2017 at 2:28 PM, Selva Nair  wrote:

> On Mon, Jun 12, 2017 at 2:14 PM, Gert Doering  wrote:
>
>> Hi,
>>
>> wading through my heap of mails that did not get proper attention...
>>
>> On Fri, May 05, 2017 at 02:24:02PM -0400, selva.n...@gmail.com wrote:
>> > From: Selva Nair 
>> >
>> > If static challenge is in use, the password passed to the plugin by
>> openvpn
>> > is of the form "SCRV1:base64-pass:base64-response". Parse this string
>> to
>> > separate it into password and response and use them to respond to
>> queries
>> > in the pam conversation function.
>> >
>> > On the plugin parameters line the substitution keyword for the static
>> > challenge response is "OTP". For example, for pam config named "test"
>> that
>> > prompts for "user", "password" and "pin", use
>> >
>> > plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin
>> OTP"
>>
>> What is the status of this one?  Does it need updating after 1/2 got
>> changed to v2 and v3 are these independent enough that 2/2 is standalone?
>>
>> From a cursory glance, it calls secure_memzero() which is now, I think,
>> plugin_secure_memzero() - right?
>>
>>
>> I also seem to remember discussions between you and David regarding
>> base64 function exporting - what's the state on this?
>>
>
>
> I have a version 2 that uses exported base64 but waiting on the following
> patch to get an NAK or ACK to finalize it.
>
>https://www.mail-archive.com/openvpn-devel@lists.sourcefo
> rge.net/msg14655.html
>
> That would also help David's base64 export patch revised, reviewed and
> merged. For background, see the thread
>
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge
> .net/msg14577.html
>

bump :)
--
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] win32 build: check for ENETUNREACH

[Selva Nair]

On Wed, Jul 12, 2017 at 10:45 AM, Илья Шипицин  wrote:

> 2017-07-12 18:54 GMT+05:00 Selva Nair :
>
>>
>> On Wed, Jul 12, 2017 at 4:46 AM, Илья Шипицин 
>> wrote:
>>
>>> No interest ?
>>>
>>> 9 июл. 2017 г. 19:46 пользователь "Ilya Shipitsin" 
>>> написал:
>>>
>>>> Currently, we do not check for mingw-gcc version. For example,
>>>> Debian 7 is shipped with 4.6.3, which does not know about ENETUNREACH
>>>> ---
>>>>
>>>> this is for 2.4 and HEAD branches
>>>>
>>>>  configure.ac | 14 ++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/configure.ac b/configure.ac
>>>> index 60bb465..80b26ff 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -708,6 +708,20 @@ m4_define(
>>>> [setsockopt getsockopt getsockname poll]dnl
>>>>  )
>>>>  if test "${WIN32}" = "yes"; then
>>>> +
>>>> +#
>>>> +# check for ENETUNREACH
>>>> +# at least Debian 7 is shipped with gcc-mingw-w64-4.6.3, which is too
>>>> old
>>>
>>>
>> Instead of checking for a particular define why not require a minimal
>> version of gcc ? I do not know what version that would be, though..
>>
>
>
> if you would try to build using ubuntu-14.04, build would fail (I'll
> submit configure patch for that soon as well).
> however, if modified compiler would used https://community.openvpn.net/
> openvpn/wiki/PatchingDebs ... the build will succeed
>

There is no reliable way to support arbitrarily patched build environments.
If I patch mingw in ubuntu 14-04 to have ENETUNREACH defined, the above
check would pass but build may still fail somewhere else.

I still think we should just determine what official version of mingw-gcc
is required and add a test for it.

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] win32 build: check for ENETUNREACH

[Selva Nair]

Hi,

On Thu, Jul 20, 2017 at 4:36 AM, Илья Шипицин  wrote:

> any news ?
>
> 2017-07-12 20:50 GMT+05:00 Илья Шипицин :
>
>>
>>
>> 2017-07-12 20:06 GMT+05:00 Selva Nair :
>>
>>>
>>> On Wed, Jul 12, 2017 at 10:45 AM, Илья Шипицин 
>>> wrote:
>>>
>>>> 2017-07-12 18:54 GMT+05:00 Selva Nair :
>>>>
>>>>>
>>>>> On Wed, Jul 12, 2017 at 4:46 AM, Илья Шипицин 
>>>>> wrote:
>>>>>
>>>>>> No interest ?
>>>>>>
>>>>>> 9 июл. 2017 г. 19:46 пользователь "Ilya Shipitsin" <
>>>>>> chipits...@gmail.com> написал:
>>>>>>
>>>>>>> Currently, we do not check for mingw-gcc version. For example,
>>>>>>> Debian 7 is shipped with 4.6.3, which does not know about ENETUNREACH
>>>>>>> ---
>>>>>>>
>>>>>>> this is for 2.4 and HEAD branches
>>>>>>>
>>>>>>>  configure.ac | 14 ++
>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/configure.ac b/configure.ac
>>>>>>> index 60bb465..80b26ff 100644
>>>>>>> --- a/configure.ac
>>>>>>> +++ b/configure.ac
>>>>>>> @@ -708,6 +708,20 @@ m4_define(
>>>>>>> [setsockopt getsockopt getsockname poll]dnl
>>>>>>>  )
>>>>>>>  if test "${WIN32}" = "yes"; then
>>>>>>> +
>>>>>>> +#
>>>>>>> +# check for ENETUNREACH
>>>>>>> +# at least Debian 7 is shipped with gcc-mingw-w64-4.6.3, which is
>>>>>>> too old
>>>>>>
>>>>>>
>>>>> Instead of checking for a particular define why not require a minimal
>>>>> version of gcc ? I do not know what version that would be, though..
>>>>>
>>>>
>>>>
>>>> if you would try to build using ubuntu-14.04, build would fail (I'll
>>>> submit configure patch for that soon as well).
>>>> however, if modified compiler would used https://community.openvpn.net/
>>>> openvpn/wiki/PatchingDebs ... the build will succeed
>>>>
>>>
>>> There is no reliable way to support arbitrarily patched build
>>> environments. If I patch mingw in ubuntu 14-04 to have ENETUNREACH
>>> defined, the above check would pass but build may still fail somewhere else.
>>>
>>
>>
>> ENETUNREACH check is for Debian 7.
>>
>> Ubuntu 14.04 is shipped with broken fwpmu.h:
>> https://github.com/chipitsine/openvpn/commit/03013fd6f5b57a1
>> 22302ce1a534a7061acde9d5b  :)
>>
>>
>>>
>>> I still think we should just determine what official version of mingw-
>>> gcc is required and add a test for it.
>>>
>>> Selva
>>>
>>
>
I still think we should just determine what official version of mingw-gcc
is required and add a test for it. :) If its just one define missing add it
to sources as we already do for some fwpm defines in block_dns.c

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] TAP-adapter iftype question

[Selva Nair]

Hi,

On Wed, Jul 26, 2017 at 1:28 PM, Karl Mueller  wrote:

>
> >
> > I have no idea what the effect would be ("will it stop doing ethernet
> > framing?  no more ARP?"), but it's an interesting idea to try.
> >
>
> Thanks, seems OpenVPN is not the only VPN client that may recognize this
> as an issue and change their iftype. https://quickview.cloudapps.
> cisco.com/quickview/bug/CSCut51057 The Cisco adapter previously used 0x6
> judging by an old Cisco VPN installation I had access to.
>
> Is it possible to have someone create a signed test build of the TAP
> adapter with iftype 131 in the oemvista.inf?
>

This should be easy to test by editing the installed OemVista.inf  (in
C:\Program Files\Tap-Windows\driver\ by default) and running addtap to
create a new adapter with the required iftype. The resulting interface will
most likely not show up in the adapter list in control panel, but should
show in 'openvpn --show-adapters'

But I do not think it will work: with iftype = 0x83 (131) the interface
will not get a physical address (MAC) etc. Further, the tap-windows driver
source sets the iftype as IF_TYPE_ETHERNET_CSMACD (=0x06) which is supposed
to match the value in the inf file.

I have a couple of wifi only win10 machines on which I have not seen this
disconnection behaviour.  Is it because I do not use redirect-gateway? Does
the wifi disconnection happen only when default route is overwritten or is
even "--redirect-gateway def1" not immune to it?

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] TAP-adapter iftype question

[Selva Nair]

Hi,

On Thu, Jul 27, 2017 at 2:01 PM, Karl Mueller  wrote:

>
> I think it is due to the redirect-gateway, and def1 does not change the
> behavior. I believe it’s because Windows sends NCSI internet probes to
> determine if an adapter has “Internet” access. If you’re not redirecting
> your gateway, those probes will probably fail via the VPN interface so it
> won’t consider the VPN interface as a viable alternative.
>

If so just redirecting msftncsi.com through VPN will trigger this?


>
> A frustrating part of the problem is that Windows overrides the routing
> table by *silently* directing new connections to the “better” Ethernet
> interface (in anticipation of the WiFi losing signal altogether, I
> suppose).  The only way to observe this is to use the powershell command
> ‘find-netroute -remote ’ which tells you where Windows will *really*
> send the packet. Also the effect of the redirection isn’t immediate,
> Windows waits from 30-120 seconds to start redirecting traffic to the VPN
> interface once it detects weak WiFi signal.
>

So to reproduce this the wifi signal has to go weak as well?


>
> Some more details of my setup: I use the Windows service-based VPN
> connection rather than user-initiated by way of openvpnserv2.exe.  We are
> using build 1511 of Windows 10. I posted my configs here:
> https://forums.openvpn.net/viewtopic.php?f=4&t=24499


I suppose you mean the opposite: that is the connection is started at boot
using OpenVPNService (i.e., openvpnserv2.exe). User initiated connections
(using the GUI) are started by OpenVPNServiceInteractive (i.e.,
openvpnserv.exe).

Will try to reproduce, but being affected only when auto-started at boot is
puzzling. In both cases the routes are set up the same way so it should not
matter.  Except for --redirect-gateway: the interactive service will force
the use of def1, but again your config specifies def1 so the behaviour
should be the same.

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] OVPN Interactive Service for non-admin users

[Selva Nair]

Hi Simon,

Adding to what I wrote in my reply to your private email:


> I am developing an eduVPN client for Windows. Imagine the eduVPN client as
> a custom OpenVPN GUI. The client uses openvpn.exe for connecting, the
> configuration file is provided by eduVPN server once user authenticates
> using OAuth. User running the eduVPN client is not an administrator.
> Elevation is out of the question.
>
>
>
> I would like to use the Interactive Service to start openvpn.exe, but I
> have some problems:
>
>
>
> 1.   The configuration file is dynamically downloaded by the eduVPN
> client and stored somewhere user can write (user's temporary folder for
> example). But the Interactive Service was specifically programmed to allow
> configurations from "C:\Program Files\OpenVPN\config" folder only. But user
> running eduVPN client can't write to this folder.
>
> 2.   Interactive Service can launch openvpn.exe using any
> configuration file if user is a member of the "OpenVPN Administrators"
> group. Then, I would need to add all users of the computer to that group,
> again requiring elevation.
>
>
>
> Is there any specific reason, why Interactive Service is so paranoid,
> knowing that it launches openvpn.exe and all external scripts as the
> interactive user anyway?
>

The service does privileged operations so some admin has to bless a user to
allow certain options when launching openvpn.exe. In other words, options
allowed in user editable configs are restricted unless the user is in a
designated group.

An admin installing openvpn can change this behaviour by customizing the
ovpn_admin_group and/or by adding users to that group.


>
>
> I have a work-around for this paradox in my sleeve: the eduVPN setup shall
> create an "eduVPN" subfolder in the "C:\Program Files\OpenVPN\config"
> folder, and grant all users desirable permissions*: a sort of public spool
> folder.
>

Setting up such a folder requires admin rights. If your installer has admin
rights, just add all users to "OpenVPN Administrators" group or set the
registry key ovpn_admin_group to "Users"


>
>
> But that would open the OpenVPN Interactive Service to any user and
> application. This is why we would like your opinion first.
>

Yes the service will then launch openvpn with arbitrary configs as any
user, but that is what you want isn't it?

Regards,

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] OVPN Interactive Service for non-admin users

[Selva Nair]

Hi,


>  But that would open the OpenVPN Interactive Service to any user and
> application. This is why we would like your opinion first.
>
> Yes the service will then launch openvpn with arbitrary configs as any
> user, but that is what you want isn't it?
>
>
>
> True, I want that indeed. I was just trying to find the official way of
> doing it only to learn it's against OpenVPN team's principles. :(
>

The official way is to add the user to the designated group which by
default is expected to be named "OpenVPN Administrators". Recursive group
membership will work, so you could create a group named, say, "eduVPN
Users" or just use "Users" and add that to "OpenVPN Administrators" group
at install time (and remove it on uninstall). Personally I would avoid
tweaking permissions of a folder inside "Program Files\OpenVPN\config\"

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] OVPN Interactive Service for non-admin users

[Selva Nair]

Hi,

On Fri, Aug 11, 2017 at 6:21 AM, Pasi Kärkkäinen  wrote:

> Hi,
>
> On Wed, Aug 09, 2017 at 02:31:58PM +, Simon Rozman via Openvpn-devel
> wrote:
> >Hi!
> >
> >I am developing an eduVPN client for Windows. Imagine the eduVPN
> client as
> >a custom OpenVPN GUI. The client uses openvpn.exe for connecting, the
> >configuration file is provided by eduVPN server once user
> authenticates
> >using OAuth. User running the eduVPN client is not an administrator.
> >Elevation is out of the question.
> >
> >
> >I would like to use the Interactive Service to start openvpn.exe, but
> I
> >have some problems:
> >
> >
> >1.   The configuration file is dynamically downloaded by the
> eduVPN
> >client and stored somewhere user can write (user's temporary folder
> for
> >example). But the Interactive Service was specifically programmed to
> allow
> >configurations from "C:\Program Files\OpenVPN\config" folder only. But
> >user running eduVPN client can't write to this folder.
> >
>
>
> Wasn't this changed in the latest version, allowing config files to be
> under user home/profile directory?
>
>
The change you are referring to is that OpenVPN-GUI now looks for configs
in the global location and in user's profile with the latter given priority
in case of duplicates.

However, to use the interactive service, config could be in any directory
only if the user is a member of (i) Administrators group OR (ii) a custom
group (named "OpenVPN Administrators" by default). Otherwise only configs
in the pre-defined global location are allowed[*]. This is done to make
sure that admins has control over who is allowed to manipulate routes etc
using the interactive service. Note that only group membership is needed,
the group need not be enabled in the token which means elevation is not
required.

Selva

[*] This actual requirement is a bit more relaxed than that as some limited
options are allowed in user-editable configs or command line for all users.
--
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 5/5] use NULL instead of 0 when assigning pointers

[Selva Nair]

On Fri, Aug 11, 2017 at 5:07 AM, Antonio Quartulli  wrote:

> From: Antonio Quartulli 
>
> Signed-off-by: Antonio Quartulli 
> ---
>  src/openvpn/ps.c | 2 +-
>  src/openvpn/ssl_openssl.c| 2 +-
>  src/openvpn/ssl_verify_openssl.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
> index 45e24ded..5136a20c 100644
> --- a/src/openvpn/ps.c
> +++ b/src/openvpn/ps.c
> @@ -922,7 +922,7 @@ port_share_open(const char *host,
>  openvpn_close_socket(fd[1]);
>
>  exit(0);
> -return 0; /* NOTREACHED */
> +return NULL; /* NOTREACHED */

 }
>
>  error:
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index fa06f068..c977b9e2 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -703,7 +703,7 @@ tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO
> *bio)
>  for (;; )
>  {
>  cert = NULL;
> -if (!PEM_read_bio_X509(bio, &cert, 0, NULL)) /* takes ownership
> of cert */
> +if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes
> ownership of cert */
>  {
>  break;
>  }
> diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_
> openssl.c
> index ea474955..2f3b10b9 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -202,8 +202,8 @@ extract_x509_field_ssl(X509_NAME *x509, const char
> *field_name, char *out,
>  {
>  int lastpos = -1;
>  int tmp = -1;
> -X509_NAME_ENTRY *x509ne = 0;
> -ASN1_STRING *asn1 = 0;
> +X509_NAME_ENTRY *x509ne = NULL;
> +ASN1_STRING *asn1 = NULL;
>


Constant 0 or (void *) 0 assigned to a pointer are the same as NULL
pointer, so why bother?

int *a = 0; is perfectly valid in my view.

Of course,
int i = 0; int *a = i;
would be bad style and error-prone, and the compiler would warn, but not
the use of literal 0.

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] OVPN Interactive Service for non-admin users

[Selva Nair]

On Fri, Aug 11, 2017 at 10:05 AM, Simon Rozman via Openvpn-devel <
openvpn-devel@lists.sourceforge.net> wrote:

>
> But that's what I wanted in the first place, as I believe Interactive
> Service "security" scheme makes no sense.
>
> Why does OpenVPN restrict non-admin users from using Interactive Service in
> the first place, while Windows' out-of-the-box VPN connects them just fine?
> If you are afraid a malware would start connecting - they already can:
> using
> Windows' VPN.
>

AFAIK, Windows VPN can be setup without admin rights only if the connection
is not shared with other users. Thus a limited user cannot redirect traffic
of all users. In openvpn we do not have a provision for such a separation
-- at least not as yet.


>
> Flushing ARP cache, client DNS registration, and other tasks OpenVPN can't
> perform as non-admin user is a technical issue of OpenVPN running in user
> space. Not a security one. Interactive Service overcomes that, but in the
> same time it assumes it's a security sensitive issue.
>

These tasks normally require admin rights (or some privilege like Network
Configuration Operators). So admin has to decide who is allowed to do such
actions.


> This limitation can and will be turned off with one or another simple
> administrator task (performed by eduVPN setup). So, this is no biggie...
>

Yes, a simple "administrator task" is all that is required to provide extra
privileges to users. In case of interactive service its supposed to be done
at the time of installation.

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

[Openvpn-devel] [PATCH] Check whether in pull_mode before warning about previous connection blocks

[selva . nair]

From: Selva Nair 

Eliminate the confusing message that says "explicit-exit-notify is ignored by
previous  blocks" when the option is pushed.
Reported by: Eike Lohmann e.lohm...@ic3s.de
https://www.mail-archive.com/openvpn-users@lists.sourceforge.net/msg04052.html

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

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 3a5bccf..d302bd4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6110,7 +6110,7 @@ add_option(struct options *options,
 #ifdef ENABLE_OCC
 else if (streq(p[0], "explicit-exit-notify") && !p[2])
 {
-
VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION|OPT_P_EXPLICIT_NOTIFY);
+VERIFY_PERMISSION(OPT_P_GENERAL|(pull_mode? 
0:OPT_P_CONNECTION)|OPT_P_EXPLICIT_NOTIFY);
 if (p[1])
 {
 options->ce.explicit_exit_notification = positive_atoi(p[1]);
-- 
2.6.2


--
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] Check whether in pull_mode before warning about previous connection blocks

[selva . nair]

From: Selva Nair 

In particular, this eliminates the message that says "explicit-exit-notify
is ignored by previous  blocks" when the option is pushed.

Note: pull_mode is identified as "allowed & OPT_P_PULL_MODE" matching with the
definition in add_options().

Reported by: Eike Lohmann e.lohm...@ic3s.de
https://www.mail-archive.com/openvpn-users@lists.sourceforge.net/msg04052.html

v2: move the check to verify_permissions() as suggested by
Gert 

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

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 3a5bccf..b4613df 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -4824,11 +4824,13 @@ verify_permission(const char *name,
 #ifndef ENABLE_SMALL
 /* Check if this options is allowed in connection block,
  * but we are currently not in a connection block
+ * unless this is a pushed option.
  * Parsing a connection block uses a temporary options struct without
  * connection_list
  */
 
-if ((type & OPT_P_CONNECTION) && options->connection_list)
+if ((type & OPT_P_CONNECTION) && options->connection_list
+&& !(allowed & OPT_P_PULL_MODE))
 {
 if (file)
 {
-- 
2.6.2


--
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] Fix missing check for return value of malloc'd buffer

[selva . nair]

From: Selva Nair 

- Use utf8to16 from common.c for utf8 to wide conversion and
  check its return value

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3f98217..f3be113 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -547,15 +547,17 @@ static DWORD
 InterfaceLuid(const char *iface_name, PNET_LUID luid)
 {
 NETIO_STATUS status;
-LPWSTR wide_name;
-int n;
-
-n = MultiByteToWideChar(CP_UTF8, 0, iface_name, -1, NULL, 0);
-wide_name = malloc(n * sizeof(WCHAR));
-MultiByteToWideChar(CP_UTF8, 0, iface_name, -1, wide_name, n);
-status = ConvertInterfaceAliasToLuid(wide_name, luid);
-free(wide_name);
+LPWSTR wide_name = utf8to16(iface_name);
 
+if (wide_name)
+{
+status = ConvertInterfaceAliasToLuid(wide_name, luid);
+free(wide_name);
+}
+else
+{
+status = ERROR_OUTOFMEMORY;
+}
 return status;
 }
 
-- 
2.6.2


--
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] Avoid illegal memory access when malformed data is read from the pipe

[selva . nair]

From: Selva Nair 

- If only 1 byte is read from the interactive service client pipe, that
  evaluates to zero wide characters and subsequent check for NUL
  termination in the data buffer segfaults.
  Fix: reject clients that send less than a complete wide character.

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index f3be113..0d162e8 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -466,6 +466,13 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 }
 
 size = bytes / sizeof(*data);
+if (size == 0)
+{
+MsgToEventLog(M_SYSERR, TEXT("malformed startup data: 1 byte 
received"));
+ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, 
&exit_event);
+goto out;
+}
+
 data = malloc(bytes);
 if (data == NULL)
 {
-- 
2.6.2


--
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] Use lowest metric interface when multiple interfaces match a route

[selva . nair]

From: Selva Nair 

Currently a route addition using IPAPI or service is skipped if the
route gateway is reachable by multiple interfaces. This changes that
to use the interface with lowest metric.

Reported by Jan Just Keijser 

Signed-off-by: Selva Nair 
---
 src/openvpn/route.c |  3 +--
 src/openvpn/tun.c   | 14 --
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6e..3937018 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2729,7 +2729,7 @@ get_default_gateway(struct route_gateway_info *rgi)
 if (rgi->gateway.addr)
 {
 rgi->flags |= RGI_ADDR_DEFINED;
-a_index = adapter_index_of_ip(adapters, rgi->gateway.addr, NULL, 
&rgi->gateway.netmask);
+a_index = dwForwardIfIndex;
 if (a_index != TUN_ADAPTER_INDEX_INVALID)
 {
 rgi->adapter_index = a_index;
@@ -2780,7 +2780,6 @@ windows_route_find_if_index(const struct route_ipv4 *r, 
const struct tuntap *tt)
 msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d matches)",
 print_in_addr_t(r->gateway, 0, &gc),
 count);
-ret = TUN_ADAPTER_INDEX_INVALID;
 }
 
 dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d index=%d",
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 3639718..d0461ef 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -45,6 +45,7 @@
 #include "manage.h"
 #include "route.h"
 #include "win32.h"
+#include "block_dns.h"
 
 #include "memdbg.h"
 
@@ -4483,6 +4484,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 struct gc_arena gc = gc_new();
 DWORD ret = TUN_ADAPTER_INDEX_INVALID;
 in_addr_t highest_netmask = 0;
+int lowest_metric = INT_MAX;
 bool first = true;
 
 if (count)
@@ -4496,9 +4498,11 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 
 if (is_ip_in_adapter_subnet(list, ip, &hn))
 {
+int metric = get_interface_metric(list->Index, AF_INET);
 if (first || hn > highest_netmask)
 {
 highest_netmask = hn;
+lowest_metric = metric;
 if (count)
 {
 *count = 1;
@@ -4512,16 +4516,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 {
 ++*count;
 }
+if (metric < lowest_metric)
+{
+ret = list->Index;
+lowest_metric = metric;
+}
 }
 }
 list = list->Next;
 }
 
-dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
+dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d 
metric=%d",
  print_in_addr_t(ip, 0, &gc),
  print_in_addr_t(highest_netmask, 0, &gc),
  (int)ret,
- count ? *count : -1);
+ count ? *count : -1,
+ metric);
 
 if (ret == TUN_ADAPTER_INDEX_INVALID && count)
 {
-- 
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] Use lowest metric interface when multiple interfaces match a route

[selva . nair]

From: Selva Nair 

Currently a route addition using IPAPI or service is skipped if the
route gateway is reachable by multiple interfaces. This changes that
to use the interface with lowest metric. Implemented by

(i)  Do not over-write the return value with TUN_ADAPTER_INDEX_INVALID in
 windows_route_find_if_index() if multiple interfaces match a route.
(ii) Select the interface with lowest metric in adapter_index_of_ip()
 instead of the first one found when multiple interfaces match.

Reported by Jan Just Keijser 

v2: - A private get_interface_metric() method and better error reporting
- Revert an unintented edit of route.c (a_index = ...)
- Improve the commit message

Signed-off-by: Selva Nair 
---
 src/openvpn/route.c |  1 -
 src/openvpn/tun.c   | 59 +++--
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6e..66a8ae3 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2780,7 +2780,6 @@ windows_route_find_if_index(const struct route_ipv4 *r, 
const struct tuntap *tt)
 msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d matches)",
 print_in_addr_t(r->gateway, 0, &gc),
 count);
-ret = TUN_ADAPTER_INDEX_INVALID;
 }
 
 dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d index=%d",
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 3639718..7603133 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -4474,6 +4474,49 @@ is_ip_in_adapter_subnet(const IP_ADAPTER_INFO *ai, const 
in_addr_t ip, in_addr_t
 return ret;
 }
 
+/**
+ * Given an interface index return the interface metric.
+ *
+ * Arguments:
+ *   index : The index of the interface
+ *   family: AF_INET for IPv4 or AF_INET6 for IPv6
+ * On error returns -1
+ */
+
+/* function signature missing in mingw iphlpapi.h */
+VOID NETIOAPI_API_
+InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
+
+static int
+get_interface_metric(NET_IFINDEX index, ADDRESS_FAMILY family)
+{
+DWORD err;
+int msglevel = D_ROUTE|M_WARN;
+MIB_IPINTERFACE_ROW ipiface;
+
+InitializeIpInterfaceEntry(&ipiface);
+ipiface.Family = family;
+ipiface.InterfaceIndex = index;
+
+err = GetIpInterfaceEntry(&ipiface);
+if (err == NO_ERROR)
+{
+return ipiface.Metric;
+}
+else if (err == ERROR_NOT_FOUND)
+{
+/*
+ *  This happens if the address family is not enabled for the
+ *  interface, which is benign -- display only at a debug level
+ */
+msglevel = D_ROUTE_DEBUG;
+}
+msg(msglevel, "Note: failed to determine metric of interface "
+  "<%lu> for %s : (error code = %lu)",
+  index, (family == AF_INET)? "ipv4" : "ipv6", err);
+return -1;
+}
+
 DWORD
 adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 const in_addr_t ip,
@@ -4483,6 +4526,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 struct gc_arena gc = gc_new();
 DWORD ret = TUN_ADAPTER_INDEX_INVALID;
 in_addr_t highest_netmask = 0;
+int lowest_metric = INT_MAX;
 bool first = true;
 
 if (count)
@@ -4496,9 +4540,14 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 
 if (is_ip_in_adapter_subnet(list, ip, &hn))
 {
+int metric = get_interface_metric(list->Index, AF_INET);
 if (first || hn > highest_netmask)
 {
 highest_netmask = hn;
+if (metric >= 0)
+{
+lowest_metric = metric;
+}
 if (count)
 {
 *count = 1;
@@ -4512,16 +4561,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 {
 ++*count;
 }
+if (metric >= 0 && metric < lowest_metric)
+{
+ret = list->Index;
+lowest_metric = metric;
+}
 }
 }
 list = list->Next;
 }
 
-dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
+dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d 
metric=%d",
  print_in_addr_t(ip, 0, &gc),
  print_in_addr_t(highest_netmask, 0, &gc),
  (int)ret,
- count ? *count : -1);
+ count ? *count : -1,
+ lowest_metric);
 
 if (ret == TUN_ADAPTER_INDEX_INVALID && count)
 {
-- 
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 2/2] Make most registry values optional

[selva . nair]

From: Selva Nair 

Not all installations need registry values such as log_dir and
config_dir especially if automatic service is not in use.
This patch provides reasonable defaults for registry values.

- Read the default value of HKLM\Software\PACKAGE_NAME to get the
  install path and construct defaults for exe_path, config_dir,
  log_dir from it. Use "ovpn", "0", NORMAL_PRIORITY as the defaults
  for config file extension, log-append flag and process priority.

The only remaining required registry entry is the root key (usually
HKLM\Software\OpenVPN) whose default value should be set to the
installation path.

Signed-off-by: Selva Nair 
---
 src/openvpnserv/common.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index bc99584..759e6a0 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -87,6 +87,8 @@ GetOpenvpnSettings(settings_t *s)
 TCHAR append[2];
 DWORD error;
 HKEY key;
+TCHAR install_path[MAX_PATH];
+TCHAR default_value[MAX_PATH];
 
 LONG status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, REG_KEY, 0, KEY_READ, &key);
 if (status != ERROR_SUCCESS)
@@ -95,37 +97,50 @@ GetOpenvpnSettings(settings_t *s)
 return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key 
HKLM\\%s not found"), REG_KEY);
 }
 
-error = GetRegString(key, TEXT("exe_path"), s->exe_path, 
sizeof(s->exe_path), NULL);
+/* The default value of REG_KEY is the install path */
+if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != 
ERROR_SUCCESS)
+{
+goto out;
+}
+
+openvpn_sntprintf(default_value, _countof(default_value), 
TEXT("%s\\bin\\openvpn.exe"),
+  install_path);
+error = GetRegString(key, TEXT("exe_path"), s->exe_path, 
sizeof(s->exe_path), default_value);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("config_dir"), s->config_dir, 
sizeof(s->config_dir), NULL);
+openvpn_sntprintf(default_value, _countof(default_value), 
TEXT("%s\\config"), install_path);
+error = GetRegString(key, TEXT("config_dir"), s->config_dir, 
sizeof(s->config_dir),
+ default_value);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("config_ext"), s->ext_string, 
sizeof(s->ext_string), NULL);
+error = GetRegString(key, TEXT("config_ext"), s->ext_string, 
sizeof(s->ext_string),
+ TEXT(".ovpn"));
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), 
NULL);
+openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\log"), 
install_path);
+error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), 
default_value);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("priority"), priority, sizeof(priority), 
NULL);
+error = GetRegString(key, TEXT("priority"), priority, sizeof(priority),
+ TEXT("NORMAL_PRIORITY_CLASS"));
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("log_append"), append, sizeof(append), 
NULL);
+error = GetRegString(key, TEXT("log_append"), append, sizeof(append), 
TEXT("0"));
 if (error != ERROR_SUCCESS)
 {
 goto out;
-- 
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 1/2] Ensure strings read from registry are null-terminated

[selva . nair]

From: Selva Nair 

- Strings stored in registry are not guaranteed to be null-terminated.
  So, use RegGetValue() instead of RegQueryValueEx() as the former
  adds null termination to the returned string if missing.
  (Needs Windows Vista+)

- While at it also add a default value parameter to GetRegString()
  to process optional registry values (such as ovpn_admin_group)
  without causing an otherwise confusing error logged to the
  eventlog[*].

[*] see Trac: #892

Signed-off-by: Selva Nair 
---
 src/openvpnserv/common.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index e77d7ab..bc99584 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -56,14 +56,18 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, 
...)
 #define REG_KEY  TEXT("SOFTWARE\\" PACKAGE_NAME)
 
 static DWORD
-GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size)
+GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR 
default_value)
 {
-DWORD type;
-LONG status = RegQueryValueEx(key, value, NULL, &type, (LPBYTE) data, 
&size);
+LONG status = RegGetValue(key, NULL, value, 
RRF_RT_REG_SZ|RRF_RT_REG_EXPAND_SZ,
+  NULL, (LPBYTE) data, &size);
 
-if (status == ERROR_SUCCESS && type != REG_SZ)
+if (status == ERROR_FILE_NOT_FOUND && default_value)
 {
-status = ERROR_DATATYPE_MISMATCH;
+size_t len = size/sizeof(data[0]);
+if (openvpn_sntprintf(data, len, default_value) > 0)
+{
+status = ERROR_SUCCESS;
+}
 }
 
 if (status != ERROR_SUCCESS)
@@ -91,48 +95,48 @@ GetOpenvpnSettings(settings_t *s)
 return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key 
HKLM\\%s not found"), REG_KEY);
 }
 
-error = GetRegString(key, TEXT("exe_path"), s->exe_path, 
sizeof(s->exe_path));
+error = GetRegString(key, TEXT("exe_path"), s->exe_path, 
sizeof(s->exe_path), NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("config_dir"), s->config_dir, 
sizeof(s->config_dir));
+error = GetRegString(key, TEXT("config_dir"), s->config_dir, 
sizeof(s->config_dir), NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("config_ext"), s->ext_string, 
sizeof(s->ext_string));
+error = GetRegString(key, TEXT("config_ext"), s->ext_string, 
sizeof(s->ext_string), NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir));
+error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), 
NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("priority"), priority, sizeof(priority));
+error = GetRegString(key, TEXT("priority"), priority, sizeof(priority), 
NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
-error = GetRegString(key, TEXT("log_append"), append, sizeof(append));
+error = GetRegString(key, TEXT("log_append"), append, sizeof(append), 
NULL);
 if (error != ERROR_SUCCESS)
 {
 goto out;
 }
 
 /* read if present, else use default */
-error = GetRegString(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, 
sizeof(s->ovpn_admin_group));
+error = GetRegString(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group,
+ sizeof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
 if (error != ERROR_SUCCESS)
 {
-openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), 
OVPN_ADMIN_GROUP);
-error = 0; /* this error is not fatal */
+goto out;
 }
 /* set process priority */
 if (!_tcsicmp(priority, TEXT("IDLE_PRIORITY_CLASS")))
-- 
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] Correct the declaration of handle in 'struct openvpn_plugin_args_open_return'

[selva . nair]

From: Selva Nair 

- This is an opaque pointer so the change should not affect
  existing plugins. But it makes the code consistent and clears up
  the documentation as the handle pointer is treated as of type
  "openvpn_plugin_handle_t" in the rest of the code.

Signed-off-by: Selva Nair 
---
 v2: no change

 include/openvpn-plugin.h.in | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index f29b3a0..dfc8e1e 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -355,7 +355,7 @@ struct openvpn_plugin_args_open_in
  *  type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_CONNECT)
  * | 
OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_DISCONNECT)
  *
- * *handle :Pointer to a global plug-in context, created by the plug-in.  
This pointer
+ * handle : Pointer to a global plug-in context, created by the plug-in.  
This pointer
  *  is passed on to the other plug-in calls.
  *
  * return_list : used to return data back to OpenVPN.
@@ -364,7 +364,7 @@ struct openvpn_plugin_args_open_in
 struct openvpn_plugin_args_open_return
 {
 int type_mask;
-openvpn_plugin_handle_t *handle;
+openvpn_plugin_handle_t handle;
 struct openvpn_plugin_string_list **return_list;
 };
 
@@ -386,9 +386,9 @@ struct openvpn_plugin_args_open_return
  *these variables are not actually written to the "official"
  *environmental variable store of the process.
  *
- * *handle : Pointer to a global plug-in context, created by the plug-in's 
openvpn_plugin_open_v3().
+ * handle : Pointer to a global plug-in context, created by the plug-in's 
openvpn_plugin_open_v3().
  *
- * *per_client_context : the per-client context pointer which was returned by
+ * per_client_context : the per-client context pointer which was returned by
  *openvpn_plugin_client_constructor_v1, if defined.
  *
  * current_cert_depth : Certificate depth of the certificate being passed over 
(only if compiled with ENABLE_CRYPTO defined)
-- 
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] Suggestion: openvpn-GUI --help style

[Selva Nair]

Hi

On Thu, Nov 23, 2017 at 1:34 PM, fragmentux  wrote:
>
> Hi,
>
> I would like to suggest that, instead of having to run the GUI to
> retrieve the help, like so:
>
> 'C:\Program Files\Openvpn\bin\openvpn-gui --help'
>
> the 'help window' can be retrieved via the GUI itself.
> A menu option or Help button ..
>
> Otherwise, because teh GUI is set on install to run by default at user
> logon it is not possible to run another instance to retrieve the help.
> Unless you quit the running GUI first.

Yeah, that's bad. But, a help menu option in the GUI may raise
expectations of a proper documentation that describes how to use
the GUI, not just a list of a command line options. Until "someone" (not me
:)
writes a doc describing the usage, including scripts, script timeouts,
default config/log/script locations etc., a help menu item would be
premature.

We could just allow running a second instance with  --help. The gui
PR #188 implements a more user-friendly handling of a second instance,
ways to send commands to the running GUI etc, so its easy to add
such a feature on top of it.

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

[Openvpn-devel] Follow up on sending messages to the GUI

[Selva Nair]

Cross-posting to users and devel as this may be of interest to both.

Hi,

I have made a draft implementation of this feature that was discussed in a
previous thread. A test executable (GUI only) is in this pre-release:

https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg

It would be great if anyone can test this out[*].

Thanks,

Selva

[*] Although virtually any text can be sent, some familiarity with openvpn
config/ccd parsing/quoting and push processing is necessary to get it right
for non-trivial messages that contain comment characters, commas, new lines
etc... Short and simple messages must be easy, though.

Also see some technical notes included in the above link.
--
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] Follow up on sending messages to the GUI

[Selva Nair]

Hi Jon,

On Thu, Nov 30, 2017 at 8:41 PM, Jonathan K. Bullard 
wrote:

> Thanks, Selva,
>
> On Wed, Nov 29, 2017 at 9:03 PM, Selva Nair  wrote:
> >
> > I have made a draft implementation of this feature that was discussed in
> a previous thread. A test executable (GUI only) is in this pre-release:
> >
> > https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg
> 
> >
> > Also see some technical notes included in the above link.
>
> The technical notes say
>
>  In case of "echo msg text" the previous text, if any, is appended
> with a new line. An empty text in this case will just add a new line.
>
>
That was a poor way of saying that "echo msg text" will display a single
line of text, not two lines as would be expected if an '\n' is automatically
appended.


> Is that the same as
>
>  In case of "echo msg text" the previous text, if any, is appended
> with text and then a new line. An empty text in this case will just
> add a new line.
>
>
Yes, that would be a less confusing way to word it provided an
additional remark is added: that the last newline so added is ignored during
display (see below).


> ? (That is, the new line comes after the text, not after the previous
> text.)
>

Well, here is what I thought should happen:

echo msg Text
echo msg-window title

should display "Text" as a single line (ignoring any "soft" linebreaks
added by the UI for display purposes). For this to happen no '\n' should
get appended to "Text".

echo msg Text1
echo msg Text2
echo msg-window title

leads to "Text1\nText2"  displayed in 2 lines

Alternatively the one could say "msg Text" implicitly appends a newline
character(s) to "Text" except that the last implicit newline does not cause
an
empty line to be displayed.

Isn't that the expected behaviour?

Selva

P.S: In reality I add about a line of blank space at the top and bottom
before display
for aesthetic reasons, but that is beside the point.
--
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] Follow up on sending messages to the GUI

[Selva Nair]

Hi,

On Fri, Dec 1, 2017 at 8:53 AM, Arne Schwabe  wrote:

> Am 30.11.2017 um 03:03 schrieb Selva Nair:
>
> Cross-posting to users and devel as this may be of interest to both.
>
> Hi,
>
> I have made a draft implementation of this feature that was discussed in a
> previous thread. A test executable (GUI only) is in this pre-release:
>
> https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg
>
> It would be great if anyone can test this out[*].
>
> Thanks,
>
> Selva
>
> [*] Although virtually any text can be sent, some familiarity with openvpn
> config/ccd parsing/quoting and push processing is necessary to get it
> right for non-trivial messages that contain comment characters, commas, new
> lines etc... Short and simple messages must be easy, though.
>
>
> Could we have some text stating that clients might only display one
> message per connect? At the moment you can have multiple "echo msg-notify
> message-title" pushed by the server. I would like to avoid in my client to
> implement logic to display multiple messages. If one message allowed the
> message can become just an Android notification without special logic
>

Yes, we can and probably should document that some clients may only
display one message. Do you also want to say that some clients may
interpret msg-window as msg-notify?

Even in case of Windows desktop, I think it may be better to display
only one message per connection as otherwise it starts to get very
noisy. At most one message window and one notification.

Jon, do you plan to document the proposed "echo msg" specs in management
notes or elsewhere?  The single message per connect limitation
could be specified there.


> > msg-window : displayed only when starting from a disconnected state
> > not when reconnecting by SIGUSR1 or SIGHUP
>
> We need to clarify this a bit. You probably mean "reconnecting after a
> successful" connection. If there are multiple remote in the config and only
> the 3rd works you already have had two internal SIGUSR1.
>

Ah, I didn't think of that. In my current implementation, it'll miss the
message if a second remote succeeds after a failure with first remote.
I'll fix this.

If a successful connection moves to a new remote, we
may get a new message worth showing to the user but will be ignored
(unless its a notification). This may not be ideal but I want to avoid
displaying popups when the user may not be even at the desktop.
Any thoughts on this?

Thanks,

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] Follow up on sending messages to the GUI

[Selva Nair]

Hi

On Sat, Dec 2, 2017 at 7:08 AM, Jonathan K. Bullard 
wrote:

> Hi,
>
> On Fri, Dec 1, 2017 at 10:58 AM, Selva Nair  wrote:
> >
> > Hi,
> >
> > On Fri, Dec 1, 2017 at 8:53 AM, Arne Schwabe  wrote:
> >>
>
..

> >>
> >> Could we have some text stating that clients might only display one
> message per connect? At the moment you can have multiple "echo msg-notify
> message-title" pushed by the server. I would like to avoid in my client to
> implement logic to display multiple messages. If one message allowed the
> message can become just an Android notification without special logic
> >
> >
> > Yes, we can and probably should document that some clients may only
> > display one message. Do you also want to say that some clients may
> > interpret msg-window as msg-notify?
> >
> > Even in case of Windows desktop, I think it may be better to display
> > only one message per connection as otherwise it starts to get very
> > noisy. At most one message window and one notification.
> >
> > Jon, do you plan to document the proposed "echo msg" specs in management
> > notes or elsewhere?  The single message per connect limitation
> > could be specified there.
>
> I'll be happy to try to document the protocol between OpenVPN and the
> GUI, including the "msg*" commands and others such as
> "forget-passwords", "setenv", etc., which we've discussed. However,
> I'm thinking it should be a separate "doc/gui-notes.txt" document.
>

Agreed, echo as such is already documented in management-notes,
and this is about standardizing how gui's interpret the echoed text
or directives.

Make sense to have a separate document.

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 v2] ifconfig-ipv6(-push): allow using hostnames

[Selva Nair]

Hi,

On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli  wrote:

> Similarly to ifconfig(-push), its IPv6 counterpart is now able to
> accept hostnames as well instead of IP addresses in numeric form.
>

If dns names currently work for ifconfig-push (I didn't know),  makes sense
to
support it for ipv6 as well.

But getaddrinfo can take a long time to timeout, so we are adding another
potentially blocking call on the server. Should not be an issue on a well
administered server, but just saying...

Otherwise looks good except for a couple of issues as below:


> Basically this means that the user is now allowed to specify
> something like this:
>
> ifconfig-ipv6-push my.hostname.cx/64
>
> This is exactly the same behaviour that we already have with
> ifconfig(-push).
>
> The generic code introduced in this patch will be later used to
> implement the /bits parsing support for IPv4 addresses.
>
> Trac: #808
> Signed-off-by: Antonio Quartulli 
> ---
>
> v2:
> - rebased on top of master
> - style adapted to new CodingStyle
>
>  src/openvpn/options.c |  61 
>  src/openvpn/options.h |   4 --
>  src/openvpn/socket.c  | 126 ++
> +++-
>  src/openvpn/socket.h  |  12 +
>  4 files changed, 126 insertions(+), 77 deletions(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 8e5cdf7f..767cdaeb 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel,
> bool *error)
>  return ret;
>  }
>
> -/* helper: parse a text string containing an IPv6 address + netbits
> - * in "standard format" (2001:dba::/32)
> - * "/nn" is optional, default to /64 if missing
> - *
> - * return true if parsing succeeded, modify *network and *netbits
> - */
> -bool
> -get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
> -   unsigned int *netbits, int msglevel)
> -{
> -char *sep, *endp;
> -int bits;
> -struct in6_addr t_network;
> -
> -sep = strchr( prefix_str, '/' );
> -if (sep == NULL)
> -{
> -bits = 64;
> -}
> -else
> -{
> -bits = strtol( sep+1, &endp, 10 );
> -if (*endp != '\0' || bits < 0 || bits > 128)
> -{
> -msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec",
> prefix_str);
> -return false;
> -}
> -}
> -
> -/* temporary replace '/' in caller-provided string with '\0',
> otherwise
> - * inet_pton() will refuse prefix string
> - * (alternative would be to strncpy() the prefix to temporary buffer)
> - */
> -
> -if (sep != NULL)
> -{
> -*sep = '\0';
> -}
> -
> -if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
> -{
> -msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address",
> prefix_str);
> -return false;
> -}
> -
> -if (sep != NULL)
> -{
> -*sep = '/';
> -}
> -
> -if (netbits != NULL)
> -{
> -*netbits = bits;
> -}
> -if (network != NULL)
> -{
> -*network = t_network;
> -}
> -return true;/* parsing OK, values set */
> -}
> -
>  /**
>   * Returns newly allocated string containing address part without "/nn".
>   *
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 035c6d15..d67c2785 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -817,8 +817,4 @@ void options_string_import(struct options *options,
> unsigned int *option_types_found,
> struct env_set *es);
>
> -bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
> -unsigned int *netbits, int msglevel );
> -
> -
>  #endif /* ifndef OPTIONS_H */
> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
> index 0fc91f21..4cadae23 100644
> --- a/src/openvpn/socket.c
> +++ b/src/openvpn/socket.c
> @@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,
>  /*
>   * Functions related to the translation of DNS names to IP addresses.
>   */
> +static int
> +get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
> + void *network, unsigned int *netbits,
> + int resolve_retry_seconds, volatile int *signal_received,
> + int msglevel)
> +{
> +char *endp, *sep, *var_host;
> +uint8_t bits, max_bits;
> +struct addrinfo *ai;
> +int ret = -1;
> +
> +ASSERT(hostname);
> +
> +/* assign family specific default values */
> +switch (af)
> +{
> +case AF_INET:
> +bits = 0;
> +max_bits = sizeof(in_addr_t) * 8;
> +break;
> +case AF_INET6:
> +bits = 64;
> +max_bits = sizeof(struct in6_addr) * 8;
> +break;
> +default:
> +ASSERT(0);
> +}
> +
> +/* we need to modify the hostname received as input, but we don't
> want to
> +  

[Openvpn-devel] Fwd: [PATCH v2] ifconfig-ipv6(-push): allow using hostnames

[Selva Nair]

oops forgot to cc the list..

-- Forwarded message --
From: Selva Nair 
Date: Sat, Dec 2, 2017 at 10:16 PM
Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using
hostnames
To: Antonio Quartulli 


Hi,

On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli  wrote:

> Hi,
>
> On 03/12/17 04:27, Selva Nair wrote:
>
> >> +/* we need to modify the hostname received as input, but we don't
> >> want to
> >> + * touch it directly as it might be a constant string.
> >> + *
> >> + * Therefore, we clone the string here and free it at the end of
> the
> >> + * function */
> >> +var_host = strdup(hostname);
> >> +ASSERT(var_host);
> >>
> >
> > I think ASSERT should be used only to catch errors in coding logic,
> > not for plausible runtime errors like this. Especially since this happens
> > on the server, no reason to terminate the process here.
> >
> > Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
> > or goto out. The option parser will log a generic warning, but still
> useful
> > to log here using M_ERRNO for more specific info.
> >
>
> I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted
> this chunk of code from somewhere else (and consider that this patch is
> quite old even though I have re-sent it now).
>
> I will get rid of the ASSERTs.
>
> > Alternatively one could use a buffer on the stack --  easy to do as good
> > old dns names are limited in size (255 octets max?) and the current
> option
> > parser also limits the argument passed here to < 255 bytes. But if we
> > support
> > internationalized domain names (currently we do not, do we?) and if the
> line
> > length in option parser is ever increased, a much larger buffer would be
> > needed.
>
> Are you sure we have a limit of 255 octects? I am not so sure. Anyway,
> this is not extremely important for now.
>

Yes, not important for this patch and stdup is anyway fine as long as we do
not
ASSERT.

The 255 byte limit comes from the config file or option parser which reads
the
input from ccd or client-connect script output. The length of a line is
limited
to  #define OPTION_LINE_SIZE 256 in options.h


> >
> >
> >> +
> >> +/* check if this hostname has a /bits suffix */
> >> +sep = strchr(var_host , '/');
> >> +if (sep)
> >> +{
> >> +bits = strtoul(sep + 1, &endp, 10);
> >
> >
> > There are a number of such type coercions in the patch
> > (ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka
> MSVC:)
> > may warn about, but I do not personally care. All are safe and deliberate
> > except for the nitpicking below.
> >
> > +if ((*endp != '\0') || (bits > max_bits))
> >
> >
> > That (bits > max_bits) check will not catch many input errors, as the
> > input is  already truncated to uint8_t. For example, /255 will be flagged
> > as an error, but /256 will pass as 0.
> >
>
> These are probably all copy/paste from other parts of the code. So the
> logic here is also used in other parts of the code. If we believe this
> is not the proper way to handle these cases, I'd suggest to take a note
> and fix these behaviors with a dedicated patch.
>

Please bear with me: Quoting from the patch:

-char *sep, *endp;
-int bits;
-struct in6_addr t_network;

So, the original code had bits defined as int, so why change to unit8_t?
Changing the original int to unsigned int would've made sense
as we are parsing using strtoul and returning the result in an unsigned int.
But uint8_t brings in hardly any gain and causes all those regressions and
type conversions I pointed out..

> Though its not possible to catch all input errors, keeping bits as
> unsigned
> > int (instead of uint8_t) may be better here. That'll  also match netbits
> > in the function signature, return value of strtoul etc..


Regards,

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 v2] ifconfig-ipv6(-push): allow using hostnames

[Selva Nair]

Hi,

Responding to this old version just to be on record.

I realized patch this was assigned to Gert on patchwork too late after
started responding on my own. Sorry for jumping the gun. Have to make
keeping an eye on patchwork a habit..

I'll leave the latest v4 alone.

cheers,

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 v2] openvpnserv: Add support for multi-instances

[Selva Nair]

Hi Simon,

IIRC, this patch is waiting for a new version to take care of the static
const as
agreed below:

On Thu, Nov 9, 2017 at 11:12 AM, Selva  wrote:

> Hi Simon,
>
> On Thu, Nov 9, 2017 at 3:33 AM, Simon Rozman  wrote:
>
>> Hi,
>>
>> > But then making the variable static just to keep a valid pointer beyond
>> the
>> > current block local looks like a kludge. For me seeing static applied
>> to a
>> > variable scoped to a block is just confusing and unusual style. Think
>> of this: if
>> > you remove that static the code may still build and even work on some
>> > compilers depending on optimization level etc. and mysteriously fail on
>> some
>> > occasions. From that one could either conclude a static qualifier is
>> required
>> > her or the variable is wrongly scoped. I think the latter conclusion is
>> the 'right'
>> > one and static is a misuse.
>> >
>> > As for the combination 'static const ...', it has a place and that is
>> for constants
>> > defined outside functions to limit the scope of an otherwise  global
>> const to
>> > that of the compilation unit.
>> >
>> > It may be just me.
>>
>> Not necessarily. It may be just *me*. :)
>>
>> Anyway, you got me convinced and I shall move those structs from data
>> segment to stack in the next version of the patch.
>
>
>  Glad that I don't have to invent an Acked-with-reservations: tag :)
>

I suppose a v4 is coming.

Thanks,

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 v2] openvpnserv: Review MSVC down-casting warnings

[Selva Nair]

Hi Simon,

And this one:

On Mon, Nov 13, 2017 at 11:26 AM, Selva  wrote:

> Hi,
>
> Thanks for the v2
>
> On Mon, Nov 13, 2017 at 4:49 AM, Simon Rozman  wrote:
>
>> Data size arithmetic was reviewed according to 64-bit MSVC complaints.
>>
>> The warnings were addressed by migrating to size_t, rewriting the code,
>> or silencing the warnings by an explicit cast where appropriate.
>> ---
>>  src/openvpnserv/automatic.c   | 20 
>>  src/openvpnserv/interactive.c | 12 ++--
>>  2 files changed, 10 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
>> index 75c3be2..4d5501b 100644
>> --- a/src/openvpnserv/automatic.c
>> +++ b/src/openvpnserv/automatic.c
>> @@ -115,25 +115,13 @@ close_if_open(HANDLE h)
>>  static bool
>>  match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>>  {
>> -int i;
>> -
>>  if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>>  {
>>  return false;
>>  }
>>
>> -if (!_tcslen(ext))
>> -{
>> -return true;
>> -}
>> -
>> -i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
>> -if (i < 1)
>> -{
>> -return false;
>> -}
>> -
>> -return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i +
>> 1, ext);
>> +const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));
>> +return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;
>
>
> FWIW, ensuring the extension string is not empty seems to be required to
> avoid a regression (see my reply to v1). A check for ext[0] != TEXT('\0')
> should do the trick.
>
> The rest looks good.
>

Needs a v3, I suppose.

Thanks,

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 v3] openvpnserv: Review MSVC down-casting warnings

[Selva Nair]

Hi Simon,

Thanks. The v3 has just arrived in patchwork -- for some reason not in my
mailbox yet, probably its coming..

Looks like v3 is an exact copy of v2 -- no check for empty ext which was
the only change required.

Am I missing something?

Thanks,

Selva

On Sun, Dec 3, 2017 at 12:19 PM, Simon Rozman  wrote:

> Data size arithmetic was reviewed according to 64-bit MSVC complaints.
>
> The warnings were addressed by migrating to size_t, rewriting the code,
> or silencing the warnings by an explicit cast where appropriate.
> ---
>  src/openvpnserv/automatic.c   | 20 
>  src/openvpnserv/interactive.c | 12 ++--
>  2 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 75c3be2..4d5501b 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -115,25 +115,13 @@ close_if_open(HANDLE h)
>  static bool
>  match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  {
> -int i;
> -
>  if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>  {
>  return false;
>  }
>
> -if (!_tcslen(ext))
> -{
> -return true;
> -}
> -
> -i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
> -if (i < 1)
> -{
> -return false;
> -}
> -
> -return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i +
> 1, ext);
> +const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));
> +return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;
>  }
>
>  /*
> @@ -142,14 +130,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  static bool
>  modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
>  {
> -int i;
> +size_t i;
>
>  if (size > 0 && (_tcslen(src) + 1) <= size)
>  {
>  _tcscpy(dest, src);
>  dest [size - 1] = TEXT('\0');
>  i = _tcslen(dest);
> -while (--i >= 0)
> +while (i-- > 0)
>  {
>  if (dest[i] == TEXT('\\'))
>  {
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index ed4603e..5d58ceb 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count,
> LPHANDLE events)
>  swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
>  buf[_countof(buf) - 1] = '\0';
>
> -WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);
> +WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
>  }
>
>  static VOID
> @@ -308,7 +308,7 @@ ReturnError(HANDLE pipe, DWORD error, LPCWSTR func,
> DWORD count, LPHANDLE events
>  L"0x%1!08x!\n%2!s!\n%3!s!", 0, 0,
>  (LPWSTR) &result, 0, (va_list *) args);
>
> -WritePipeAsync(pipe, result, wcslen(result) * 2, count, events);
> +WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count,
> events);
>  #ifdef UNICODE
>  MsgToEventLog(MSG_FLAGS_ERROR, result);
>  #else
> @@ -452,10 +452,10 @@ out:
>  static BOOL
>  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
>  {
> -size_t len;
> +size_t size, len;
>  BOOL ret = FALSE;
>  WCHAR *data = NULL;
> -DWORD size, bytes, read;
> +DWORD bytes, read;
>
>  bytes = PeekNamedPipeAsync(pipe, 1, &exit_event);
>  if (bytes == 0)
> @@ -1051,7 +1051,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t
> *proto, const wchar_t *if_nam
>  const wchar_t *fmt = L"netsh interface %s %s dns \"%s\" %s";
>
>  /* max cmdline length in wchars -- include room for worst case and
> some */
> -int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;
> +size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 +
> 1;
>  wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
>  if (!cmdline)
>  {
> @@ -1571,7 +1571,7 @@ RunOpenvpn(LPVOID p)
>  {
>  DWORD written;
>  WideCharToMultiByte(CP_UTF8, 0, sud.std_input, -1, input,
> input_size, NULL, NULL);
> -WriteFile(stdin_write, input, strlen(input), &written, NULL);
> +WriteFile(stdin_write, input, (DWORD)strlen(input), &written,
> NULL);
>  free(input);
>  }
>
> --
> 2.9.0.windows.1
>
> Hi Selva,
>
> I have git-send-email v3 of this patch on November 13th but it got lost
> somewhere along the way. :( I can see it in the SMTP log, but didn't notice
> it hasn't appeared on the mailing list.
>
> Here you go again. I hope this time it makes it thru.
>
> Best regards,
> Simon
>
> 
> --
> 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] Suggestion: openvpn-GUI --help style

[Selva Nair]

Hi,

On Thu, Nov 23, 2017 at 6:59 PM, Selva Nair  wrote:

> Hi
>
> On Thu, Nov 23, 2017 at 1:34 PM, fragmentux  wrote:
> >
> > Hi,
> >
> > I would like to suggest that, instead of having to run the GUI to
> > retrieve the help, like so:
> >
> > 'C:\Program Files\Openvpn\bin\openvpn-gui --help'
> >
> > the 'help window' can be retrieved via the GUI itself.
> > A menu option or Help button ..
> >
> > Otherwise, because teh GUI is set on install to run by default at user
> > logon it is not possible to run another instance to retrieve the help.
> > Unless you quit the running GUI first.
>
> Yeah, that's bad. But, a help menu option in the GUI may raise
> expectations of a proper documentation that describes how to use
> the GUI, not just a list of a command line options. Until "someone" (not
> me :)
> writes a doc describing the usage, including scripts, script timeouts,
> default config/log/script locations etc., a help menu item would be
> premature.
>
> We could just allow running a second instance with  --help. The gui
> PR #188 implements a more user-friendly handling of a second instance,
> ways to send commands to the running GUI etc, so its easy to add
> such a feature on top of it.
>

 A followup to say that, in fact, PR #188 as is will allow
running openvpn-gui --help even whhen an instance is already running.

This is not to say a help menu is of no interest --- I would very much like
to
have help documentation accessible from the GUI. But it has to show
something
useful to an average user -- not a bunch of command line arguments.

Writing docs is one thing that even non-programmers can contribute to. We
could
link to (or distribute with the GUI) an html page like
https://community.openvpn.net/openvpn/wiki/OpenVPN-GUI if that is brought
in line with the current state of the GUI. Several things like need for
admin rights,
config file locations etc. are totally outdated in there. Also show how to
import
configs which is the first thing a user would would want to do etc...

Any volunteers to bring it up to date or point to something better?

Thanks,

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 v2] ifconfig-ipv6(-push): allow using hostnames

[Selva Nair]

On Sun, Dec 3, 2017 at 1:54 PM, Gert Doering  wrote:

> Hi,,
>
> On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote:
> > Responding to this old version just to be on record.
> >
> > I realized patch this was assigned to Gert on patchwork too late after
> > started responding on my own. Sorry for jumping the gun. Have to make
> > keeping an eye on patchwork a habit..
> >
> > I'll leave the latest v4 alone.

If that is the consequence of assigning patches to people on patchwork,
> we should stop doing so (Antonio, are you listening? :-) ).


I didn't want to step on anyone's toes... But based on Antonio's
explanation,
I take that remark back. Will look into v4 as soon as I get some time to
test it.


> Obviously
> the patch was in dire need of a good review, and if you have time while
> I am busy with family things, this is more than welcome.
>
> Voluntarily *taking* a patch for review is useful as a flag "I'm going to
> handle this", but third-party-tagging "this is for Gert!" seems to have
> potential for backfiring, so maybe we shouldn't do that, then.


Agreed. I just didn't realize Antonio delegated it to you and assumed
all "delegation" are indications that the person has volunteered to look
into it. As for me, I totally welcome others reviewing something I delegated
to myself, or commenting/criticizing/extoling (or tipping :) my reviews
(multiple reviews are indeed great).

But I wouldn't particularly want others delegating work to me :)
So let's continue the practice of delegating to self, not to others.
We can always use the list (or IRC for those who frequent the channel)
for requesting reviews etc.

Best,

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 v5] openvpnserv: Review MSVC down-casting warnings

[Selva Nair]

Hi,

This looks good now.

The v1 of this patch and main review comments are in a different thread:
https://sourceforge.net/p/openvpn/mailman/openvpn-devel/
thread/20171010231130.6832-12-simon%40rozman.si/#msg36071613

On Sun, Dec 3, 2017 at 3:30 PM, Simon Rozman  wrote:

> Data size arithmetic was reviewed according to 64-bit MSVC complaints.
>
> The warnings were addressed by migrating to size_t, rewriting the code,
> or silencing the warnings by an explicit cast where appropriate.
> ---
>  src/openvpnserv/automatic.c   | 17 ++---
>  src/openvpnserv/interactive.c | 12 ++--
>  2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 75c3be2..6f82b4e 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -115,25 +115,20 @@ close_if_open(HANDLE h)
>  static bool
>  match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  {
> -int i;
> -
>  if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>  {
>  return false;
>  }
>
> -if (!_tcslen(ext))
> +if (*ext == TEXT('\0'))
>  {
>  return true;

 }
>

Empty extension matching all files is somewhat strange but that's the
original behaviour (unlike I initially thought), and is preserved by the
patch.


>
> -i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
> -if (i < 1)
> -{
> -return false;
> -}
> +/* find the pointer to that last '.' in filename and match ext
> against the rest */
>
> -return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i +
> 1, ext);
> +const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));
> +return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;
>  }
>
>  /*
> @@ -142,14 +137,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  static bool
>  modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
>  {
> -int i;
> +size_t i;
>
>  if (size > 0 && (_tcslen(src) + 1) <= size)
>  {
>  _tcscpy(dest, src);
>  dest [size - 1] = TEXT('\0');
>  i = _tcslen(dest);
> -while (--i >= 0)
> +while (i-- > 0)
>  {
>  if (dest[i] == TEXT('\\'))
>  {
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index ed4603e..5d58ceb 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count,
> LPHANDLE events)
>  swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
>  buf[_countof(buf) - 1] = '\0';
>
> -WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);
> +WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
>  }
>
>  static VOID
> @@ -308,7 +308,7 @@ ReturnError(HANDLE pipe, DWORD error, LPCWSTR func,
> DWORD count, LPHANDLE events
>  L"0x%1!08x!\n%2!s!\n%3!s!", 0, 0,
>  (LPWSTR) &result, 0, (va_list *) args);
>
> -WritePipeAsync(pipe, result, wcslen(result) * 2, count, events);
> +WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count,
> events);
>  #ifdef UNICODE
>  MsgToEventLog(MSG_FLAGS_ERROR, result);
>  #else
> @@ -452,10 +452,10 @@ out:
>  static BOOL
>  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
>  {
> -size_t len;
> +size_t size, len;
>  BOOL ret = FALSE;
>  WCHAR *data = NULL;
> -DWORD size, bytes, read;
> +DWORD bytes, read;
>
>  bytes = PeekNamedPipeAsync(pipe, 1, &exit_event);
>  if (bytes == 0)
> @@ -1051,7 +1051,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t
> *proto, const wchar_t *if_nam
>  const wchar_t *fmt = L"netsh interface %s %s dns \"%s\" %s";
>
>  /* max cmdline length in wchars -- include room for worst case and
> some */
> -int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;
> +size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 +
> 1;
>  wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
>  if (!cmdline)
>  {
> @@ -1571,7 +1571,7 @@ RunOpenvpn(LPVOID p)
>  {
>  DWORD written;
>  WideCharToMultiByte(CP_UTF8, 0, sud.std_input, -1, input,
> input_size, NULL, NULL);
> -WriteFile(stdin_write, input, strlen(input), &written, NULL);
> +WriteFile(stdin_write, input, (DWORD)strlen(input), &written,
> NULL);
>  free(input);
>  }
>

Cleaner and shorter code and should silence some pesky MSVC
warnings.

Acked-by: Selva Nair 
--
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 v4] ifconfig-ipv6(-push): allow using hostnames

[Selva Nair]

INET, flags, hostname, &addr, NULL,
> +  resolve_retry_seconds, signal_received,
> +  M_WARN);
>  if (status==0)
>  {
> -struct in_addr ia;
>  if (succeeded)
>  {
>  *succeeded = true;
>  }
> -ia = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
> -freeaddrinfo(ai);
> -return (flags & GETADDR_HOST_ORDER) ? ntohl(ia.s_addr) : ia.s_addr;
> +return addr;
>  }
>  else
>  {
> @@ -112,6 +215,19 @@ getaddr(unsigned int flags,
>  }
>  }
>
> +bool
> +get_ipv6_addr(const char *hostname, struct in6_addr *network,
> +  unsigned int *netbits, int msglevel)
> +{
> +if (get_addr_generic(AF_INET6, GETADDR_RESOLVE, hostname, network, 
> netbits,
> + 0, NULL, msglevel) < 0)
> +{
> +return false;
> +}
> +
> +return true;    /* parsing OK, values set */
> +}
> +
>  static inline bool
>  streqnull(const char *a, const char *b)
>  {
> diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
> index 2d7f2187..81e9e9ae 100644
> --- a/src/openvpn/socket.h
> +++ b/src/openvpn/socket.h
> @@ -532,12 +532,24 @@ bool unix_socket_get_peer_uid_gid(const 
> socket_descriptor_t sd, int *uid, int *g
>
>  #define GETADDR_CACHE_MASK  (GETADDR_DATAGRAM|GETADDR_PASSIVE)
>
> +/**
> + * Translate an IPv4 addr or hostname from string form to in_addr_t
> + *
> + * In case of resolve error, it will try again for
> + * resolve_retry_seconds seconds.
> + */
>  in_addr_t getaddr(unsigned int flags,
>const char *hostname,
>int resolve_retry_seconds,
>bool *succeeded,
>volatile int *signal_received);
>
> +/**
> + * Translate an IPv6 addr or hostname from string form to in6_addr
> + */
> +bool get_ipv6_addr(const char *hostname, struct in6_addr *network,
> +   unsigned int *netbits, int msglevel);
> +
>  int openvpn_getaddrinfo(unsigned int flags,
>  const char *hostname,
>  const char *servname,

Looks good and works as advertised. Thanks.

Reviewed-by: Selva Nair 
Tested-by:  Selva Nair 
Acked-by: Selva Nair 

--
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 v4] openvpnserv: Add support for multi-instances

[Selva Nair]

Hi,

v4 looks good and passes my tests and "opinionated" demands.

Thanks, Simon.

On Sun, Dec 3, 2017 at 4:16 PM, Simon Rozman  wrote:
>
> While openvpn.exe can run multiple concurrent processes, openvpnserv.exe
> is usually only one single globally unique running process.
>
> This patch extends openvpnserv.exe to support multiple service instances
> in parallel allowing side-by-side OpenVPN installations.
>
> Alternate instances must be installed as `SERVICE_WIN32_OWN_PROCESS`
> (Type 0x10) and must use the newly introduced service command line
> parameter:
> -instance  
>  can be `automatic` or `interactive`.
>
> - The service settings will be loaded from `HKLM\Software\OpenVPN`
>   registry key.
>
> - The automatic service will use `openvpn_exit_1` exit event.
>
> - The interactive service will accept requests on
>   `\\.\pipe\openvpn\service` named pipe, and run IPC with
>   openvpn.exe on `\\.\pipe\openvpn\service_`.
>
> This patch preserves backward compatibility, by defaulting to
> `SERVICE_WIN32_SHARE_PROCESS` and `` as service ID.
> ---


Although this is a new feature, the original behaviour is unaffected and
the possibility of named instances will help projects like eduVPN use
official binary releases.

So, I recommend this for 2.4 as well..

Reviewed by: SelvaNair 
Acked-by: Selva Nair 

--
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] Refactor get_interface_metric to return metric and auto flag separately

[selva . nair]

From: Selva Nair 

- Instead of returning metric = 0 when automatic metric is in use
  return the actual metric and flag automatic metric through a
  parameter. This makes the function reusable elsewhere.

- Ensure return value can be correctly cast to int and return -1 on
  error.

Signed-off-by: Selva Nair 
---
 src/openvpn/block_dns.c   | 30 --
 src/openvpn/block_dns.h   | 14 +++---
 src/openvpn/win32.c   | 15 +++
 src/openvpnserv/interactive.c | 13 +++--
 4 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
index d43cbcf..8d5011d 100644
--- a/src/openvpn/block_dns.c
+++ b/src/openvpn/block_dns.c
@@ -344,33 +344,43 @@ delete_block_dns_filters(HANDLE engine_handle)
 }
 
 /*
- * Returns interface metric value for specified interface index.
+ * Return interface metric value for the specified interface index.
  *
  * Arguments:
  *   index : The index of TAP adapter.
  *   family: Address family (AF_INET for IPv4 and AF_INET6 for IPv6).
- * Returns positive metric value or zero for automatic metric on success,
- * a less then zero error code on failure.
+ *   is_auto   : On return set to true if automatic metric is in use.
+ *   Unused if NULL.
+ *
+ * Returns positive metric value or -1 on error.
  */
-
 int
-get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family, int 
*is_auto)
 {
 DWORD err = 0;
 MIB_IPINTERFACE_ROW ipiface;
 InitializeIpInterfaceEntry(&ipiface);
 ipiface.Family = family;
 ipiface.InterfaceIndex = index;
+
+if (is_auto)
+{
+*is_auto = 0;
+}
 err = GetIpInterfaceEntry(&ipiface);
-if (err == NO_ERROR)
+
+/* On Windows metric is never > INT_MAX so return value of int is ok.
+ * But we check for overflow nevertheless.
+ */
+if (err == NO_ERROR && ipiface.Metric <= INT_MAX)
 {
-if (ipiface.UseAutomaticMetric)
+if (is_auto)
 {
-return 0;
+*is_auto = ipiface.UseAutomaticMetric;
 }
-return ipiface.Metric;
+return (int)ipiface.Metric;
 }
-return -err;
+return -1;
 }
 
 /*
diff --git a/src/openvpn/block_dns.h b/src/openvpn/block_dns.h
index c9a9d70..50b383f 100644
--- a/src/openvpn/block_dns.h
+++ b/src/openvpn/block_dns.h
@@ -39,17 +39,17 @@ add_block_dns_filters(HANDLE *engine, int iface_index, 
const WCHAR *exe_path,
   block_dns_msg_handler_t msg_handler_callback);
 
 /**
- * Returns interface metric value for specified interface index.
+ * Return interface metric value for the specified interface index.
  *
- * @param index The index of TAP adapter
- * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
+ * @param index The index of TAP adapter.
+ * @param familyAddress family (AF_INET for IPv4 and AF_INET6 for 
IPv6).
+ * @param is_auto   On return set to true if automatic metric is in use.
+ *  Unused if NULL.
  *
- * @return positive metric value or zero for automatic metric on success,
- * a less then zero error code on failure.
+ * @return positive interface metric on success or -1 on error
  */
-
 int
-get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family);
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family, int 
*is_auto);
 
 /**
  * Sets interface metric value for specified interface index.
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 95fea5d..4b20298 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1344,17 +1344,16 @@ win_wfp_block_dns(const NET_IFINDEX index, const HANDLE 
msg_channel)
block_dns_msg_handler);
 if (status == 0)
 {
-tap_metric_v4 = get_interface_metric(index, AF_INET);
-tap_metric_v6 = get_interface_metric(index, AF_INET6);
-if (tap_metric_v4 < 0)
+int is_auto = 0;
+tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto);
+if (is_auto)
 {
-/* error, should not restore metric */
-tap_metric_v4 = -1;
+tap_metric_v4 = 0;
 }
-if (tap_metric_v6 < 0)
+tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto);
+if (is_auto)
 {
-/* error, should not restore metric */
-tap_metric_v6 = -1;
+tap_metric_v6 = 0;
 }
 status = set_interface_metric(index, AF_INET, BLOCK_DNS_IFACE_METRIC);
 if (!status)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index aab784e..3907438 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -804,17 +804,18 @@ HandleBlockDNSMessage(const block_dns_

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi Simon,

On Tue, Dec 5, 2017 at 4:44 AM, Simon Rozman  wrote:
> Hi,
>
>> On Wed, Nov 08, 2017 at 06:46:53PM +, Simon Rozman wrote:
>> > > The best time to re-factor a function would be when a  a new use
>> > > case needs to change its semantics. Apart from the ill-chosen -err
>> > > as a return value, currently it returns 0 if automatic metric is in
>> > > use, making it impossible to use it as a generic function to find the 
>> > > current
>> metric of an interface.
>> > >
>> > > In fact I've a pending patch where such a change would help.
>> [..]
>> > I shall give it a look after the Hackathon.
>>
>> "ping"?
>>
>> This seems to be the only hunk left from the MSVC correction series (as far
>> as .c/.h files are concerned, not .proj)
>>
>
> I really appreciate your "ping" Gert. I totally forgot about this one and 
> have now flagged this thread so I shall finish it in the following weeks.
>
> The get_interface_metric() function should get a more thorough rewrite than 
> just a compiler warning shut-up. So the patch will probably get divided in 
> two - the simple signed/unsigned fixes and get_interface_metric() redesign.

For the latter, I had "re-invented" the get_interface_metric function for
the pending "use lowest metric interface when multiple interfaces match
a route" patch.

Obviously, its better to refactor this one and use it there, so I just copied
the implementation and submitted a patch to do so. Unlike I had thought
earlier, this has to stay in block_dns.h for ease of sharing with the service.

Could you please take a look and see whether it addresses MSVC's
concerns among other things?

Thanks,

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 v2] Use lowest metric interface when multiple interfaces match a route

[Selva Nair]

On Wed, Dec 6, 2017 at 8:28 AM, Илья Шипицин  wrote:
>
>
> 2017-11-06 6:14 GMT+05:00 :
>>
>>
..
>> +/**
>> + * Given an interface index return the interface metric.
>> + *
>> + * Arguments:
>> + *   index : The index of the interface
>> + *   family: AF_INET for IPv4 or AF_INET6 for IPv6
>> + * On error returns -1
>> + */
>> +
>> +/* function signature missing in mingw iphlpapi.h */
>
>
> that definition apparently present in mingw since 2015
>
> https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ea95d55e3387353e453d6ae8fc5cb8f7503947c2/tree/mingw-w64-headers/include/netioapi.h
>
>
>>
>> +VOID NETIOAPI_API_
>> +InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
>>

2015 may sound old,  but we are targeting ubuntu 16.04 which has mingw 3.1.0
from mid 2014.

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] Refactor get_interface_metric to return metric and auto flag separately

[Selva Nair]

Hi,

On Wed, Dec 6, 2017 at 12:18 PM, Simon Rozman  wrote:

> Hi,
>
> I have briefly reviewed this patch. If you look at each
> get_interface_metric() call you'll notice exactly the same repeating
> pattern:
>
> tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto);
> if (is_auto)
> {
> tap_metric_v4 = 0;
> }
> tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto);
> if (is_auto)
> {
> tap_metric_v6 = 0;
> }
>
> Should the get_interface_metric() be rewritten to return:
> 0 = automatic metric
> n = manual metric
>
>
That's exactly what the original version does and the purpose of the patch
is to not do so. Else this function is not useful when someone really wants
to
find what the metric is as the value of 0 hides that info. Note that even
when
automatic metric is in use the interface has some metric set.

I want to use it in another patch where the actual metric is required even
if automatic metric is in use.

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 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi,

On Wed, Dec 6, 2017 at 12:10 PM, Simon Rozman  wrote:
> Hi,
>
>> > The get_interface_metric() function should get a more thorough rewrite
>> than just a compiler warning shut-up. So the patch will probably get divided
>> in two - the simple signed/unsigned fixes and get_interface_metric()
>> redesign.
>>
>> For the latter, I had "re-invented" the get_interface_metric function for the
>> pending "use lowest metric interface when multiple interfaces match a
>> route" patch.
>>
>> Obviously, its better to refactor this one and use it there, so I just 
>> copied the
>> implementation and submitted a patch to do so. Unlike I had thought earlier,
>> this has to stay in block_dns.h for ease of sharing with the service.
>>
>> Could you please take a look and see whether it addresses MSVC's concerns
>> among other things?
>
> I have tested and am confirming MSVC is happy with the block_dns.c now.
>
> Should your patch be merged, I shall rebase the "[PATCH 09/13] 
> Signed/unsigned warnings of MSVC resolved" to the new master and deliver the 
> next version.

Yes, if you can review and ack/nak it :)

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] Don't throw fatal errors from verify_cert_export_cert()

[Selva Nair]

Hi,

On Fri, Dec 8, 2017 at 5:33 AM, Steffan Karger  wrote:

> From: Steffan Karger 
>
> As with create_temp_file(), this function is called on client connects and
> should not cause fatal errors when I/O (possibly temporarily) fails.
>
> The callers of this function are already fixed in the commit that does the
> same for create_temp_file().
>
> Signed-off-by: Steffan Karger 
> ---
>  src/openvpn/ssl_verify.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index ebb1da2..12cff9a 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -557,13 +557,14 @@ verify_cert_export_cert(openvpn_x509_cert_t
> *peercert, const char *tmp_dir, stru
>  peercert_file = fopen(peercert_filename, "w+");
>  if (!peercert_file)
>  {
> -msg(M_ERR, "Failed to open temporary file : %s",
> peercert_filename);
> +msg(M_ERRNO, "Failed to open temporary file : %s",
> peercert_filename);
>

I think M_ERRNO alone is not a good flag  -- if you OR it with M_NONFATAL,
syslog
will get level = LOG_ERR, management will get the 'N' flag etc.

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 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi,

On Thu, Dec 7, 2017 at 12:32 PM, Gisle Vanem  wrote:
> Simon Rozman wrote:
>
>> However, I did stare-review your code:
>> - It does not introduce any new Windows API calls it has not used before.
>> - It compiles fine.
>
> It also builds fine here with cl v19.11.
> But using clang-cl v5, I'm getting:
>   In file included from src/openvpn/argv.c:36:
>   ./src/openvpn/syshead.h(368,13) :  error: typedef redefinition with
> different types ('int' vs 'enum MIB_TCP_STATE')
>   typedef int MIB_TCP_STATE;
>   ^
>   f:\ProgramFiler-x86\WindowsKits\Include\10.0.15063.0\shared\tcpmib.h(56,3)
> :  note: previous definition is here
>   } MIB_TCP_STATE;
> ^
>   1 error generated.
>
> I fail to why I don't get this error with MSVC.
> But the lines in syshead.h should simply not assume
> MinGW is the only compiler on Windows. Line 365:
>
>   #ifdef _WIN32
>   /* Missing declarations for old-school MinGW32 or MinGW-w64 v1.x.
>*/
>   #if defined(__MINGW32__) && !(defined(__MINGW64_VERSION_MAJOR) &&
> __MINGW64_VERSION_MAJOR < 2)
>   typedef int MIB_TCP_STATE;
>   #endif

Are you referring to the patch under discussion or something else?

The code you quote above is not in current master nor in 2.4. In fact that
!(defined(__MINGW64_VERSION_MAJOR) &&__MINGW64_VERSION_MAJOR < 2
even contradicts the comment above it. Its likely not from the
official codebase.

Anyway, MIB_TCP_STATE is typedef-ed to int in syshead.h whenever
 _WIN32 is defined. We may not need it anymore, but it hasn't so
far interfered with any builds.

Indeed its an enum as per MSDN (vista+ addition?). If clang doesn't
like passing an int as enum and is using Microsoft's headers, it could
trip there even if MSVC allows it. Such a combination has never been
tested before, I suppose.

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] Follow up on sending messages to the GUI

[Selva Nair]

Hi Jon,

Thanks for starting this process.

On Thu, Dec 14, 2017 at 10:42 AM, Jonathan K. Bullard
 wrote:
> Hi,
>
> On Sat, Dec 2, 2017 at 7:08 AM, Jonathan K. Bullard  
> wrote:
>> Hi,
>>
>> On Fri, Dec 1, 2017 at 10:58 AM, Selva Nair  wrote:

...snipped..

>> I'll be happy to try to document the protocol between OpenVPN and the
>> GUI, including the "msg*" commands and others such as
>> "forget-passwords", "setenv", etc., which we've discussed. However,
>> I'm thinking it should be a separate "doc/gui-notes.txt" document.
>>
>> In a separate document it would be easier to make it clear that it is
>> describing the protocol between the configuration and the GUIs and not
>> get lost in the complexity of the management interface itself.
>
> Below is a first draft of documentation for all of the "echo" commands
> sent from OpenVPN via the management interface (typically, to a GUI).
>
> I think it's important to include info about how the each of the
> common open GUIs interpret the commands in this document, so those who
> want to use --echo will have a single place to look.
>
> I'm doing this inline in this email, not as a patch for several
> reasons: because it's easier to read that way, because I'd like to get
> it at least close to acceptance before proposing it as a patch, and
> because I'm not really proficient with OpenVPN's patching process and
> there will probably be several versions of the patch. (If someone else
> wants to do this as a patch from right now, I'm happy to have them
> take it over.)
>
> The section on quoting should be examined carefully -- I didn't test
> any of that.
>
> And I don't know which commands will be implemented on Android so I
> left that as "??".
>
> Best regards,
>
> Jon
>
>
> 
> *** New document starts after the next line ***
> 
> Management Interface "echo" protocol
>
> 
> THIS IS A PRELIMINARY VERSION OF THIS DOCUMENT. ALL INFORMATION IN IT
> IS SUBJECT TO CHANGE.
> 
>
>
> CONTENTS
> THE OPENVPN --ECHO OPTION
> ENVIRONMENT COMMAND
> MESSSAGE COMMANDS
> PASSWORD COMMANDS
> QUOTING
> COMMMAND DETAILS
>
>
> =
> THE OPENVPN --ECHO OPTION
> =
>
> The OpenVPN --echo option causes commands to be sent out through the
> management interface, typically to a Graphic User Interface (GUI) such
> as "OpenVPN for Android", "Tunnelblick" (for macOS), or "Windows
> OpenVPN GUI". It can be included in a configuration file or on a
> command line, or can be pushed from the server.
>
> This document describes the commands that can be sent and how they are
> interpreted by various GUIs.
>
>  * OpenVPN does not process the commands in an --echo option; it only
> sends them out through the management interface.
>
>  * "echo" commands are processed by the GUI if, as, when, and in the
> order they are received. If no GUI is present the processing of
> commands may be delayed, the commands may never be processed, or only
> some commands may be processed. (That can happen if OpenVPN discards
> commands because its buffer for the commands fills up.)
>
>  * There is no mechanism for the GUI to acknowledge the receipt,
> success, or failure of a command.
>
>  * "echo" commands are stored by OpenVPN (within limits, see the next
> point) and sent only when the GUI requests them through the management
> interface. "echo" commands in the configuration file or the command
> line are typically requested and processed at the start of a
> connection attempt. "echo" commands that are pushed by the server are
> also typically asked for at the start of a connection attempt but can
> be sent at any time. They are processed in the middle of a connection
> attempt or after a connection is established, as the "push" options
> are received by the client from the server.

This may require some clarification as what you are describing is the
result of "echo all" issued by the management client (GUI), but one
would also use "echo on" for real-time echo notification once
the mgmt interface is connected.

It may be useful to explain "echo on" "echo all" etc as it appears in
the  man

Re: [Openvpn-devel] [PATCH] Add common_name to the conv method. This allows the common_name to be accessible in PAM.

[Selva Nair]

Hi,

On Sat, Dec 16, 2017 at 1:57 PM, Michael Karvan
 wrote:
> ---
>  src/plugins/auth-pam/auth-pam.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index ae514d7..c64e14b 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -637,9 +637,16 @@ my_conv(int n, const struct pam_message **msg_array,
>  ret = PAM_CONV_ERR;
>  }
>  break;
> +
> +case PAM_TEXT_INFO:
> +aresp[i].resp = strdup(up->common_name);
> +if (aresp[i].resp == NULL)
> +{
> +ret = PAM_CONV_ERR;
> +}
> +break;

The purpose of PAM_TEXT_INFO is for the module to send an info message
to the user. Using it to send the common name back to the module is
hackish. Yes, it can work in a custom module but its not right to
interpret every
PAM_TEXT_INFO msg as a request for common name.

Why not prompt for it just like username and have the plugin return it?

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 v2] Don't throw fatal errors from verify_cert_export_cert()

[Selva Nair]

Hi,

On Fri, Dec 29, 2017 at 5:18 AM, Steffan Karger
 wrote:
> As with create_temp_file(), this function is called on client connects and
> should not cause fatal errors when I/O (possibly temporarily) fails.
>
> The callers of this function are already fixed in the commit that does the
> same for create_temp_file().
>
> Signed-off-by: Steffan Karger 
> ---
> v2: Use M_NONFATAL (instead of M_WARN/M_ERRNO), as suggested by Selva.
>
>  src/openvpn/ssl_verify.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index ebb1da2..0ba9f41 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -549,7 +549,7 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
> const char *tmp_dir, stru
>  if (!tmp_dir
>  || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
>  {
> -msg (M_WARN, "Failed to create peer cert file");
> +msg(M_NONFATAL, "Failed to create peer cert file");
>  return NULL;
>  }
>
> @@ -557,13 +557,15 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
> const char *tmp_dir, stru
>  peercert_file = fopen(peercert_filename, "w+");
>  if (!peercert_file)
>  {
> -msg(M_ERR, "Failed to open temporary file : %s", peercert_filename);
> +msg(M_NONFATAL|M_ERRNO, "Failed to open temporary file: %s",
> +peercert_filename);
>  return NULL;
>  }
>
>  if (SUCCESS != x509_write_pem(peercert_file, peercert))

The openssl version of x509_write_pem() called here could fail with
M_ERR --- is that already fixed in one of the pending patches? If not,
why not make that one too non-fatal?

>  {
> -msg(M_ERR, "Error writing PEM file containing certificate");
> +msg(M_NONFATAL, "Error writing PEM file containing certificate");

Yeah, not including M_ERRNO looks like the right thing to do here.

> +peercert_filename = NULL;

This could potentially lead to a stale tempfile left behind. Could be
fixed by unlinking here? Successfully exported cert file does get unlinked
after the verify script returns.

Sorry, earlier I only made a hasty remark about the error flag and did not do
a proper review...

>  }
>
>  fclose(peercert_file);
> --

Best wishes for 2018!

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 v3] Don't throw fatal errors from verify_cert_export_cert()

[Selva Nair]

Hi,

Thanks for v3. Looks good now.

On Tue, Jan 2, 2018 at 5:52 PM, Steffan Karger
 wrote:
> As with create_temp_file(), this function is called on client connects
> and should not cause fatal errors when I/O (possibly temporarily) fails.
> Fix this and the openssl backend implementation of x509_write_pem() to
> no longer throw fatal errors.
>
> The callers of this function are already fixed in the commit that does
> the same for create_temp_file().
>
> Signed-off-by: Steffan Karger 
> ---
> v2: Use M_NONFATAL (instead of M_WARN/M_ERRNO), as suggested by Selva.
> v3: Also fix x509_write_pem and unlink file on write error, per Selva too.
>
>  src/openvpn/ssl_verify.c | 9 ++---
>  src/openvpn/ssl_verify_openssl.c | 2 +-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index ebb1da2..5ae4fbb 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -549,7 +549,7 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
> const char *tmp_dir, stru
>  if (!tmp_dir
>  || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
>  {
> -msg (M_WARN, "Failed to create peer cert file");
> +msg(M_NONFATAL, "Failed to create peer cert file");
>  return NULL;
>  }
>
> @@ -557,13 +557,16 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
> const char *tmp_dir, stru
>  peercert_file = fopen(peercert_filename, "w+");
>  if (!peercert_file)
>  {
> -msg(M_ERR, "Failed to open temporary file : %s", peercert_filename);
> +msg(M_NONFATAL|M_ERRNO, "Failed to open temporary file: %s",
> +peercert_filename);
>  return NULL;
>  }
>
>  if (SUCCESS != x509_write_pem(peercert_file, peercert))
>  {
> -msg(M_ERR, "Error writing PEM file containing certificate");
> +msg(M_NONFATAL, "Error writing PEM file containing certificate");
> +(void) platform_unlink(peercert_filename);
> +peercert_filename = NULL;
>  }
>
>  fclose(peercert_file);
> diff --git a/src/openvpn/ssl_verify_openssl.c 
> b/src/openvpn/ssl_verify_openssl.c
> index 02850fc..238292f 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -767,7 +767,7 @@ x509_write_pem(FILE *peercert_file, X509 *peercert)
>  {
>  if (PEM_write_X509(peercert_file, peercert) < 0)
>  {
> -msg(M_ERR, "Failed to write peer certificate in PEM format");
> +msg(M_NONFATAL, "Failed to write peer certificate in PEM format");
>  return FAILURE;
>  }
>  return SUCCESS;

Acked-by: Selva Nair 

--
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] openvpn segfaults on --management-external-key with ECC certificate

[Selva Nair]

Hi,

I expected an error message saying only RSA certs are supported for
--management-external-key, but openvpn appears to segfault if a cert
with an ECC key is used with that option.

A stack trace shows it fails in ssl_openssl.c line 1117 when trying to
copy n and e. In fact the call

pub_rsa = EVP_PKEY_get0_RSA(pkey);

before that (line 1104) should have failed and the code does correctly
check its return value. But that call succeeds for some reason.
Instead, RSA_get0_key() returns invalid n and e pointers and passing
those to BN_dup() fails.

This is with openssl 1.0.1 and that could be the problem -- it may not
have EVP_PKEY_get0_RSA() in which case the compatibility interface in
use is probably not smart enough...

Is this a known issue or is it just me?

Selva

P.S.

FWIW, here is where it blows up: master built with --disable-lzo and CLFAGS = -g

#0  0x773fdc49 in BN_copy () from
/usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0
#1  0x773fdd46 in BN_dup () from
/usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0
#2  0x00488932 in tls_ctx_use_external_private_key
(ctx=0x7fffded0, cert_file=0x70faa8 "sansel-ecc.crt",
cert_file_inline=0x0) at ssl_openssl.c:1117
#3  0x0047e88d in init_ssl (options=0x7fffd730,
new_ctx=0x7fffded0) at ssl.c:658
#4  0x004240e4 in do_init_crypto_tls_c1 (c=0x7fffd730) at
init.c:2514
#5  0x004244ce in do_init_crypto_tls (c=0x7fffd730,
flags=3) at init.c:2617
#6  0x00424f64 in do_init_crypto (c=0x7fffd730, flags=3)
at init.c:2866
#7  0x004271a8 in init_instance (c=0x7fffd730,
env=0x707c90, flags=4) at init.c:4083
#8  0x00426d6f in init_instance_handle_signals
(c=0x7fffd730, env=0x707c90, flags=4) at init.c:3894
#9  0x004445bc in tunnel_point_to_point (c=0x7fffd730) at
openvpn.c:91
#10 0x004449c4 in openvpn_main (argc=3, argv=0x7fffe618)
at openvpn.c:305
#11 0x00444ac8 in main (argc=3, argv=0x7fffe618) at openvpn.c:388

--
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] Return NULL if GetAdaptersInfo fails

[selva . nair]

From: Selva Nair 

- Currently a pointer to potentially uninitialized IP_ADAPTER_INFO
  struct is returned on error causing ill-defined behaviour.

Signed-off-by: Selva Nair 
---

There have been some reports of unexpected failure in GetAdaptersInfo.
When and why that happens is still unclear but this bug could explain
why the process goes into a tailspin in such occasions.

 src/openvpn/tun.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 25831ce..6e16348 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -4178,15 +4178,12 @@ get_adapter_info_list(struct gc_arena *gc)
 else
 {
 pi = (PIP_ADAPTER_INFO) gc_malloc(size, false, gc);
-if ((status = GetAdaptersInfo(pi, &size)) == NO_ERROR)
-{
-return pi;
-}
-else
+if ((status = GetAdaptersInfo(pi, &size)) != NO_ERROR)
 {
 msg(M_INFO, "GetAdaptersInfo #2 failed (status=%u) : %s",
 (unsigned int)status,
 strerror_win32(status, gc));
+pi = NULL;
 }
 }
 return pi;
-- 
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